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