RFR: 8229867: Re-examine synchronization usages in http and https protocol handlers

Daniel Fuchs dfuchs at openjdk.java.net
Thu Oct 8 15:24:44 UTC 2020


Hi,

This is a fix that upgrades the old HTTP and HTTPS legacy stack to use virtual-thread friendly locking instead of
synchronized monitors.

Most of the changes are mechanical - but there are still a numbers of subtle non-mechanical differences that are
outlined below:

1. src/java.base/share/classes/sun/net/www/MessageHeader.java:
   `MessageHeader::print(PrintStream)` => synchronization modified to not synchronize on this while printing
   ( a snapshot of the data is taken before printing instead)

2. src/java.base/share/classes/sun/net/www/MeteredStream.java:
    `MeteredStream::close` was missing synchronization: it is now protected by the lock

3. src/java.base/share/classes/sun/net/www/http/ChunkedOutputStream.java:
    `ChunkedOutputStream` no longer extends `PrintStream` but extends `OutputStream` directly.
    Extending `PrintStream` is problematic for virtual thread. After careful analysis, it appeared that
    `ChunkedOutputStream` didn't really need to extend `PrintStream`. `ChunkedOutputStream`
    is already wrapping a `PrintStream`.  `ChunkedOutputStream` is never returned directly to user
    code but is wrapped in another stream. `ChunkedOutputStream` completely reimplement and
    reverse the flush logic implemented by its old super class`PrintStream` which leads me to believe
    there was no real reason for it to extend `PrintStream` - except for being able to call its `checkError`
    method - which can be done by using `instanceof ChunkedOutputStream` in the caller instead of
    casting to PrintStream.

4. src/java.base/share/classes/sun/net/www/http/HttpClient.java:
    Synchronization removed from `HttpClient::privilegedOpenServer` and replaced by an `assert`.
    There is no need for a synchronized here as the method is only called from a method that already
    holds the lock.

5. src/java.base/share/classes/sun/net/www/http/KeepAliveCache.java:
    Synchronization removed from `KeepAliveCache::removeVector` and replaced by an `assert`.
    This method is only called from methods already protected by the lock.
    Also the method has been made private.

6. src/java.base/share/classes/sun/net/www/http/KeepAliveStream.java
    `queuedForCleanup` is made volatile as it is read and written directly from outside the class
    and without protection by the `KeepAliveCleanerEntry`.
    Lock protection is also added to `close()`, which was missing it.
    Some methods that have no lock protection and did not need it because only called from
    within code blocks protected by the lock have aquired an `assert isLockHeldByCurrentThread();`

7. Concrete subclasses of `AuthenticationInfo` that provide an implementation for
    `AuthenticationInfo::setHeaders(HttpURLConnection conn, HeaderParser p, String raw)` have
    acquired an `assert conn.isLockheldByCurrentThread();` as the method should only be called
    from within a lock-protected block in `s.n.w.p.h.HttpURLConnection`

8. src/java.base/share/classes/sun/net/www/protocol/http/HttpURLConnection.java
    Several methods in this class have a acquired an `assert isLockheldByCurrentThread();`
    to help track the fact that AuthenticationInfo::setHeaders is only called while
    holding the lock.
    Synchronization was also removed from some method that didn't need it because only
    called from within code blocks protected by the lock:
    `getOutputStream0()`: synchronization removed and replaced by an `assert isLockheldByCurrentThread();`
    `setCookieHeader()`:  synchronization removed and replaced by an `assert isLockheldByCurrentThread();`
    `getInputStream0()`:  synchronization removed and replaced by an `assert isLockheldByCurrentThread();`
    `StreamingOutputStream`: small change to accomodate point 3. above (changes in ChunkedOutputStream).

9. src/java.base/share/classes/sun/net/www/protocol/http/NegotiateAuthentication.java.:
    synchronization removed from `setHeaders` and replace by an assert. See point 7. above.

10. src/java.base/unix/classes/sun/net/www/protocol/http/ntlm/NTLMAuthentication.java:
    synchronization removed from `setHeaders` and replace by an assert. See point 7. above.

11. src/java.base/windows/classes/sun/net/www/protocol/http/ntlm/NTLMAuthentication.java:
    synchronization removed from `setHeaders` and replace by an assert. See point 7. above.

-------------

Commit messages:
 - 8229867: Re-examine synchronization usages in http and https protocol handlers

Changes: https://git.openjdk.java.net/jdk/pull/558/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=558&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8229867
  Stats: 1116 lines in 18 files changed: 551 ins; 149 del; 416 mod
  Patch: https://git.openjdk.java.net/jdk/pull/558.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/558/head:pull/558

PR: https://git.openjdk.java.net/jdk/pull/558



More information about the security-dev mailing list