On Thu, 8 Oct 2020 11:22:09 GMT, Daniel Fuchs <dfu...@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. Just a few comments.
src/java.base/share/classes/sun/net/www/MessageHeader.java line 330:
> 328: at the end. Omits pairs with a null key. Omits
> 329: colon if key-value pair is the requestline. */
> 330: private void print(int nkeys, String[] keys, String[] values,
> PrintStream p) {
While not strictly necessary, this method (along with isRequestline) could be
_static_. Which ensures that their
implementations do not access instance fields.
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?
src/java.base/share/classes/sun/net/www/http/KeepAliveStream.java line 69:
> 67: public void close() throws IOException {
> 68: // If the inputstream is closed already, or if this stream
> 69: // has already been queued for cleanup.just return.
Minor typo, "cleanup.just"
-------------
PR: https://git.openjdk.java.net/jdk/pull/558
