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

[Alan Bateman]

On Thu, 8 Oct 2020 11:22:09 GMT, Daniel Fuchs <dfuchs at openjdk.org> wrote:

> 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.

Mostly looks good.

HttpClient.openServer - is there a reason to do the SM permission check while holding the lock?

The "closed" field in MeteredStream and ChunkedInputStream needs to be volatile if you really want to read it without
holding the lock.

There are a couple of places with comments that I assume you added for yourself when auditing this code. There's one in
HttpCapture, and 3 in HttpURLConnection.

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

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