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

[Daniel Fuchs]

On Thu, 8 Oct 2020 17:14:24 GMT, Alan Bateman <alanb 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.

Thanks Alan!

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

Mostly that was a mechanical change since openServer was synchronized before.
But I guess it seems also desirable for accessing host & port which are protected and not final;

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

I am not so sure. `closed` starts at `false`, and can only moved from `false` to `true`. It can never go back to
`false` after it's been set to `true`; So if you observe `true` outside of the lock it does means that it is really
closed, isn't it?  If `closed` is not volatile, the worse that can happen is that you will observe `false` while it's
really `true`, and then you will need to pay the price of locking, after which you should be able to see the real
`true` value.

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

Yes - well - I thought that would be mostly beneficial for someone searching for `synchronized` in the java/net and
sun/net packages - so that they can determine that the `synchronized` in those places was considered safe and
intentionally kept unchanged, and not just overlooked. I can remove them or reformulate if you think it's better - but
I was actually intending to keep these comments.

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

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