Re: RFR: 8229867: Re-examine synchronization usages in http and https protocol handlers [v3]
On Mon, 12 Oct 2020 13:50:30 GMT, Daniel Fuchs 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. > > Daniel Fuchs has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev > excludes the unrelated changes brought in by the merge/rebase. The pull > request contains four additional commits since > the last revision:
Re: RFR: 8229867: Re-examine synchronization usages in http and https protocol handlers [v3]
On Mon, 12 Oct 2020 13:50:30 GMT, Daniel Fuchs 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. > > Daniel Fuchs has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev > excludes the unrelated changes brought in by the merge/rebase. The pull > request contains four additional commits since > the last revision:
Re: RFR: 8229867: Re-examine synchronization usages in http and https protocol handlers [v3]
> 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. Daniel Fuchs has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision: - 8229867: Re-examine synchronization usages in http and https protocol handlers Incorporated review feedback - Merge - 8229867: Re-examine
Re: RFR: 8229867: Re-examine synchronization usages in http and https protocol handlers [v2]
On Fri, 9 Oct 2020 13:22:08 GMT, Alan Bateman wrote: >> Daniel Fuchs has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8229867: Re-examine synchronization usages in http and https protocol >> handlers >> >> Incorporated review feedback > > src/java.base/share/classes/sun/net/www/http/HttpCapture.java line 59: > >> 57: // Although accessing files could result in blocking operations, >> 58: // HttpCapture is a corner case; there seem no urgent need to convert >> 59: // this class to using java.util.concurrent.locks at this time. > > The updated patch looks good but I think this comment needs another iteration > to ensure that it doesn't confuse future > maintainers. You could drop it or else replace it with something simple that > says that HttpCapture does blocking I/O > operations while holding monitors but it's not a concern because it rarely > used. Updated as requested. - PR: https://git.openjdk.java.net/jdk/pull/558
Re: RFR: 8229867: Re-examine synchronization usages in http and https protocol handlers [v2]
On Fri, 9 Oct 2020 11:49:30 GMT, Daniel Fuchs 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. > > Daniel Fuchs has updated the pull request incrementally with one additional > commit since the last revision: > > 8229867: Re-examine synchronization usages in http and https protocol > handlers > > Incorporated review feedback
Re: RFR: 8229867: Re-examine synchronization usages in http and https protocol handlers [v2]
On Fri, 9 Oct 2020 09:01:43 GMT, Chris Hegarty wrote: >> Daniel Fuchs has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8229867: Re-examine synchronization usages in http and https protocol >> handlers >> >> Incorporated review feedback > > Mostly looks good. Just a few comments. >From Alan: > Allowing for visibility failures here is confusing for maintainers. If you > really want to access closed without the > lock (and I don't see any reason to do that) then it would be clearer to all > if it were volatile. >From Chris: > This double check of closed is kind of irritating. Is it really need, or > maybe we just drop it and lock unconditionally? => I have removed the double locking. >From Alan: > I don't have a strong opinion here but they did initially look like left over > comments. Will they mean anything to > someone looking at this code in 2025? I have updated the comments to be more explanatory for future maintainer (especially if they use the Annotate feature of the IDE to correlate with the fix in which they were introduced). That said, if you prefer removing them altogether let me know, and I'll remove them. - PR: https://git.openjdk.java.net/jdk/pull/558
Re: RFR: 8229867: Re-examine synchronization usages in http and https protocol handlers [v2]
On Fri, 9 Oct 2020 08:49:54 GMT, Chris Hegarty wrote:
>> Daniel Fuchs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8229867: Re-examine synchronization usages in http and https protocol
>> handlers
>>
>> Incorporated review feedback
>
> 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"
done
> 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.
Done.
-
PR: https://git.openjdk.java.net/jdk/pull/558
Re: RFR: 8229867: Re-examine synchronization usages in http and https protocol handlers [v2]
> 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. Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision: 8229867: Re-examine synchronization usages in http and https protocol handlers Incorporated review feedback - Changes: - all: https://git.openjdk.java.net/jdk/pull/558/files - new: https://git.openjdk.java.net/jdk/pull/558/files/c8dc2ac9..ddfa2e6c Webrevs: - full:
Re: RFR: 8229867: Re-examine synchronization usages in http and https protocol handlers
On Fri, 9 Oct 2020 09:17:48 GMT, Daniel Fuchs wrote:
>> 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. That's a good suggestion.
I agree on that point.
-
PR: https://git.openjdk.java.net/jdk/pull/558
Re: RFR: 8229867: Re-examine synchronization usages in http and https protocol handlers
On Fri, 9 Oct 2020 08:36:03 GMT, Chris Hegarty 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
Re: RFR: 8229867: Re-examine synchronization usages in http and https protocol handlers
On Thu, 8 Oct 2020 11:22:09 GMT, Daniel Fuchs 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
Re: RFR: 8229867: Re-examine synchronization usages in http and https protocol handlers
On Thu, 8 Oct 2020 17:36:31 GMT, Daniel Fuchs wrote: > 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; Okay, I mis-read the old code. The syncrhonization to access host/port isn't obvious so leaving it as is is okay. > 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. Allowing for visibility failures here is confusing for maintainers. If you really want to access closed without the lock (and I don't see any reason to do that) then it would be clearer to all if it were volatile. > 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. I don't have. a strong opinion here but they did initially look like left over comments. Will they mean anything to someone looking at this code in 2025? - PR: https://git.openjdk.java.net/jdk/pull/558
Re: RFR: 8229867: Re-examine synchronization usages in http and https protocol handlers
On Thu, 8 Oct 2020 17:14:24 GMT, Alan Bateman 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
Re: RFR: 8229867: Re-examine synchronization usages in http and https protocol handlers
On Thu, 8 Oct 2020 11:22:09 GMT, Daniel Fuchs 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.
RFR: 8229867: Re-examine synchronization usages in http and https protocol handlers
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. - Commit messages: - 8229867: Re-examine synchronization usages in http and https protocol handlers Changes: https://git.openjdk.java.net/jdk/pull/558/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=558=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8229867 Stats: 1116 lines in 18 files changed: 551 ins; 149 del; 416 mod Patch: https://git.openjdk.java.net/jdk/pull/558.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/558/head:pull/558 PR: https://git.openjdk.java.net/jdk/pull/558
