Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8169272

Clarify Semaphore.drainPermits behavior when current permits are negative

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: P4
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 9
    • Component/s: core-libs
    • Labels:

      Description

      According to description for java.util.concurrent.Semaphore it is ok to create a Semaphore with negative amount of permits. Moreover, all Semaphore's methods (including reducePermits, release, etc) works with negative amount of permits correctly (don't give permits until amount became more than zero, etc). Except drainPermits.
      If Semaphore has less than zero permits, drainPermits returns negative value (that maybe considered as ok), but also drainPermits raises permits to zero, that is definitely an error.
      Suggested fix is quite simple:
      Current code:
             final int drainPermits() {
                  for (;;) {
                      int current = getState();
                      if (current == 0 || compareAndSetState(current, 0))
                          return current;
                  }
              }

      Fixed code should be:
             final int drainPermits() {
                  for (;;) {
                      int current = getState();
                      if (current <= 0 )
                         return 0;
                      if (compareAndSetState(current, 0))
                          return current;
                  }
              }

        Activity

        Hide
        martin Martin Buchholz added a comment -
        In any case, the documentation should be clearer about what happens when drainPermits is called when permits is negative.
        Show
        martin Martin Buchholz added a comment - In any case, the documentation should be clearer about what happens when drainPermits is called when permits is negative.
        Hide
        skuksenko Sergey Kuksenko added a comment -
        In general, negative amount of permits should be considered as absence of permits. It only has impact to how many permits we should add (release) until new permits became available. In that case docs for drainPermits is ok now. But I agree that maybe we should add a section about negative permits to overall Semaphore documentation (right now negative permits are mentioned only for constructors)
        Show
        skuksenko Sergey Kuksenko added a comment - In general, negative amount of permits should be considered as absence of permits. It only has impact to how many permits we should add (release) until new permits became available. In that case docs for drainPermits is ok now. But I agree that maybe we should add a section about negative permits to overall Semaphore documentation (right now negative permits are mentioned only for constructors)
        Hide
        dl Doug Lea added a comment -
        I agree that the current (intentional) behavior should be clarified in documentation. Just adding the following seems to suffice:

        *** 603,608 ****
        --- 603,609 ----
          
              /**
               * Acquires and returns all permits that are immediately available.
        + * Upon return, zero permits are available.
               *
               * @return the number of permits acquired
               */
        Show
        dl Doug Lea added a comment - I agree that the current (intentional) behavior should be clarified in documentation. Just adding the following seems to suffice: *** 603,608 **** --- 603,609 ----          /**        * Acquires and returns all permits that are immediately available. + * Upon return, zero permits are available.        *        * @return the number of permits acquired        */
        Hide
        hgupdate HG Updates added a comment -
        URL: http://hg.openjdk.java.net/jdk9/dev/jdk/rev/426bcf3f5b93
        User: martin
        Date: 2016-11-29 08:30:02 +0000
        Show
        hgupdate HG Updates added a comment - URL: http://hg.openjdk.java.net/jdk9/dev/jdk/rev/426bcf3f5b93 User: martin Date: 2016-11-29 08:30:02 +0000
        Hide
        hgupdate HG Updates added a comment -
        URL: http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/426bcf3f5b93
        User: lana
        Date: 2016-12-07 19:33:44 +0000
        Show
        hgupdate HG Updates added a comment - URL: http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/426bcf3f5b93 User: lana Date: 2016-12-07 19:33:44 +0000

          People

          • Assignee:
            martin Martin Buchholz
            Reporter:
            skuksenko Sergey Kuksenko
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: