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

[Alan Bateman]

On Fri, 9 Oct 2020 11:49:30 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.
>
> Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8229867: Re-examine synchronization usages in http and https protocol handlers
>   
>   Incorporated review feedback

src/java.base/share/classes/sun/net/www/http/HttpCapture.java line 59:

> 57:     // Although accessing files could result in blocking operations,
> 58:     // HttpCapture is a corner case; there seem no urgent need to convert
> 59:     // this class to using java.util.concurrent.locks at this time.

The updated patch looks good but I think this comment needs another iteration to ensure that it doesn't confuse future
maintainers. You could drop it or else replace it with something simple that says that HttpCapture does blocking I/O
operations while holding monitors but it's not a concern because it rarely used.

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

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