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. This pull request has now been integrated. Changeset: 65393a09 Author: Daniel Fuchs <dfu...@openjdk.org> URL: https://git.openjdk.java.net/jdk/commit/65393a09 Stats: 1126 lines in 18 files changed: 558 ins; 156 del; 412 mod 8229867: Re-examine synchronization usages in http and https protocol handlers Reviewed-by: chegar, alanb, michaelm ------------- PR: https://git.openjdk.java.net/jdk/pull/558
