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

java.net.http.ExecutorWrapper.synchronize() is superfluous

    XMLWordPrintable

    Details

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

      Description

      In recently committed java.net.http.ExecutorWrapper, there is a synchronize() method [1], which is used as "memory fence" [2]:

       public synchronized void synchronize() {}

       public void execute(Runnable r, Supplier<AccessControlContext>
      ctxSupplier) {
         synchronize();
         Runnable r1 = () -> {
           try {
             r.run();
           } catch (Throwable t) {
             Log.logError(t);
           }
         };

         ...

         executor.execute(r1);
       }

      How's that supposed to work? Is that supposed to guard from bad Runnable $r?

      The problem is, once you get $r via the race, there is no way to recover with local synchronization (IOW: There is no way to sanitize a racy input, once it happened. Races are bad like that) And if $r got to you properly, you don't need to do anything special too (IOW: API may as well assume it is coming from the current thread).

      Therefore, I think synchronize() method there is superfluous.

      In fact, assuming that a synchronized method has any *detached* memory semantics is wrong too -- compilers are known to elide associated fences. E.g. if ExecutorWrapper is known to never escape a thread, or a single thread locks on it, and biases a lock towards itself.

      [1]
      http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/e0da6c2a5c32/src/java.httpclient/share/classes/java/net/http/ExecutorWrapper.java#l74
      [2]
      http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/e0da6c2a5c32/src/java.httpclient/share/classes/java/net/http/ExecutorWrapper.java#l77

        Attachments

          Activity

            People

            Assignee:
            michaelm Michael McMahon
            Reporter:
            shade Aleksey Shipilev
            Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved: