On Fri, 9 Oct 2020 08:36:03 GMT, Chris Hegarty <[email protected]> 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.
>
> src/java.base/share/classes/sun/net/www/MeteredStream.java line 123:
>
>> 121: lock();
>> 122: try {
>> 123: if (closed) return -1;
>
> This double check of `closed` is kind of irritating. Is it really need, or
> maybe we just drop it and lock
> unconditionally?
We could.
-------------
PR: https://git.openjdk.java.net/jdk/pull/558
