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. 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. ------------- PR: https://git.openjdk.java.net/jdk/pull/558
