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

[Daniel Fuchs]

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.

This pull request has now been integrated.

Changeset: 65393a09
Author:    Daniel Fuchs <dfuchs at openjdk.org>
URL:       https://git.openjdk.java.net/jdk/commit/65393a09
Stats:     1126 lines in 18 files changed: 558 ins; 156 del; 412 mod

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

Reviewed-by: chegar, alanb, michaelm

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

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