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: 8252374: Add a new factory method to concatenate a sequence of BodyPublisher instances into a single publisher. [v11]
On Wed, 7 Oct 2020 13:12:33 GMT, Daniel Fuchs wrote: >> Continuing the review with a PR... >> >> 8252374: Add a new factory method to concatenate a sequence >> of BodyPublisher instances into a single publisher. >> https://bugs.openjdk.java.net/browse/JDK-8252374 >> >> >> Draft CSR: >> https://bugs.openjdk.java.net/browse/JDK-8252382 > > 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 15 additional commits since > the last revision: > - Merge > - Added a plain test for positive requests. > - Keep changes to PullPublisher for a later fix > - Fixed handling of negative request. Added BodyPublishers::concat to the > TCK tests. > - Use a dataProvider for testNegativeRequest > - Merge > - Added a test case for requesting a non positive number of items. > - Merge > - Incorporated review comments > - Improved the API documentation of BodyPublishers::concat > - ... and 5 more: > https://git.openjdk.java.net/jdk/compare/2210e3d2...9ff5a3b3 Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/57
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 met
Re: RFR: 8245194: Unix domain socket channel implementation [v17]
On Tue, 6 Oct 2020 19:53:18 GMT, Michael McMahon wrote:
>> Continuing this review as a PR on github with the comments below
>> incorporated. I expect there will be a few more
>> iterations before integrating.
>> On 06/09/2020 19:47, Alan Bateman wrote:
>>> On 26/08/2020 15:24, Michael McMahon wrote:
As I mentioned the other day, I wasn't able to use the suggested method on
Windows which returns an absolute path. So,
I added a method to WindowsPath which returns the path in the expected
UTF-8 encoding as a byte[]. Let me know what you
think of that.
There is a fair bit of other refactoring and simplification done also.
Next version is at:
http://cr.openjdk.java.net/~michaelm/8245194/impl.webrev/webrev.9/
>>> Adding a method to WindowsPath to encode the path using UTF-8 is okay but I
>>> don't think we should be caching it as the
>>> encoding for sun_path is an outlier on Windows. I'm also a bit dubious
>>> about encoding a relative path when the resolved
>>> path (before encoding) is > 247 chars. The documentation on the MS site
>>> isn't very completely and I think there are a
>>> number points that require clarification to fully understand how this will
>>> work with relative, directly relative and
>>> drive relative paths.
>>>
>>
>> Maybe it would be better to just use the path returned from toString() and
>> do the conversion to UTF-8 externally. That
>> would leave WindowsPath unchanged.
>>> In the same area, the new PathUtil is a bit inconsistent with the existing
>>> provider code. One suggestion is to add a
>>> method to AbstractFileSystemProvider instead. That is the base class the
>>> platform providers and would be a better place
>>> to get the file path in bytes.
>>>
>>
>> Okay, I gave the method a name that is specific to Unix domain sockets
>> because this UTF-8 strangeness is not likely to
>> be useful by other components.
>>> One other comment on the changes to the file system provider it should be
>>> okay to change UnixUserPrinipals ad its
>>> fromUid and fromGid method to be public. This would mean that the patch
>>> shouldn't need to add UnixUserGroup (the main
>>> issue is that class is that it means we end up with two classes with static
>>> factory methods doing the same thing).
>>
>> Okay, that does simplify it a bit.
>>
>> Thanks,
>> Michael.
>>
>>> -Alan.
>>>
>>>
>>>
>>>
>>>
>>>
>
> Michael McMahon has updated the pull request incrementally with one
> additional commit since the last revision:
>
> unixdomainchannels:
> - updated property name
> - added JFR unit test
src/java.base/share/classes/java/net/UnixDomainSocketAddress.java line 2:
> 1: /*
> 2: * Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved.
Can the copyright year range for 2020 be added please.
src/java.base/share/classes/java/net/UnixDomainSocketAddress.java line 139:
> 137:
> 138: /**
> 139: * Create a UnixDomainSocketAddress from the given path string.
Create*S* a ...
src/java.base/share/classes/java/net/UnixDomainSocketAddress.java line 156:
> 154:
> 155: /**
> 156: * Create a UnixDomainSocketAddress for the given path.
Create*S* a ...
src/java.base/share/classes/java/net/UnixDomainSocketAddress.java line 173:
> 171:
> 172: /**
> 173: * Return this address's path.
Return*S* this ...
test/jdk/java/nio/channels/unixdomain/AddressTest.java line 2:
> 1: /*
> 2: * Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved.
Can the copyright year range for 2020 be added please.
test/jdk/java/nio/channels/unixdomain/AddressTest.java line 49:
> 47: throw new RuntimeException("Expected illegal path exception");
> 48: } catch (IllegalArgumentException e) {}
> 49: }
Would be a good candidate for a testng test, using assertThrows or similar.
test/jdk/java/nio/channels/unixdomain/IOExchanges.java line 113:
> 111: SPINBAccep_NBConn_NBIO_WR_11a
> 112: SPINBAccep_NBConn_NBIO_RW_12a
> 113: */
I recognise this test ;-) I thought there was a version for IP-specific
channels, but cannot find it now. I was going
to ask if these could be merged or abstracted out somehow.
-
PR: https://git.openjdk.java.net/jdk/pull/52
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 i
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 [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: https://webrevs.openj
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]
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 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 src/java.base/share/classe
Re: RFR: 8245194: Unix domain socket channel implementation [v17]
On Fri, 9 Oct 2020 09:14:33 GMT, Chris Hegarty wrote: >> Michael McMahon has updated the pull request incrementally with one >> additional commit since the last revision: >> >> unixdomainchannels: >> - updated property name >> - added JFR unit test > > test/jdk/java/nio/channels/unixdomain/IOExchanges.java line 113: > >> 111: SPINBAccep_NBConn_NBIO_WR_11a >> 112: SPINBAccep_NBConn_NBIO_RW_12a >> 113: */ > > I recognise this test ;-) I thought there was a version for IP-specific > channels, but cannot find it now. I was going > to ask if these could be merged or abstracted out somehow. Chris, Thanks for the comments. I will incorporate them all into the next revision. As regards the IOExchanges test, it uses a @DataProvider of ProtocolFamily with values "UNIX" and "INET" which means the tests are run for TCP sockets. Is that what you were asking? I'm not sure I get the question exactly. Michael. - PR: https://git.openjdk.java.net/jdk/pull/52
Re: RFR: 8245194: Unix domain socket channel implementation [v18]
> Continuing this review as a PR on github with the comments below > incorporated. I expect there will be a few more > iterations before integrating. > On 06/09/2020 19:47, Alan Bateman wrote: >> On 26/08/2020 15:24, Michael McMahon wrote: >>> >>> As I mentioned the other day, I wasn't able to use the suggested method on >>> Windows which returns an absolute path. So, >>> I added a method to WindowsPath which returns the path in the expected >>> UTF-8 encoding as a byte[]. Let me know what you >>> think of that. >>> >>> There is a fair bit of other refactoring and simplification done also. Next >>> version is at: >>> >>> http://cr.openjdk.java.net/~michaelm/8245194/impl.webrev/webrev.9/ >>> >> Adding a method to WindowsPath to encode the path using UTF-8 is okay but I >> don't think we should be caching it as the >> encoding for sun_path is an outlier on Windows. I'm also a bit dubious about >> encoding a relative path when the resolved >> path (before encoding) is > 247 chars. The documentation on the MS site >> isn't very completely and I think there are a >> number points that require clarification to fully understand how this will >> work with relative, directly relative and >> drive relative paths. >> > > Maybe it would be better to just use the path returned from toString() and do > the conversion to UTF-8 externally. That > would leave WindowsPath unchanged. >> In the same area, the new PathUtil is a bit inconsistent with the existing >> provider code. One suggestion is to add a >> method to AbstractFileSystemProvider instead. That is the base class the >> platform providers and would be a better place >> to get the file path in bytes. >> > > Okay, I gave the method a name that is specific to Unix domain sockets > because this UTF-8 strangeness is not likely to > be useful by other components. >> One other comment on the changes to the file system provider it should be >> okay to change UnixUserPrinipals ad its >> fromUid and fromGid method to be public. This would mean that the patch >> shouldn't need to add UnixUserGroup (the main >> issue is that class is that it means we end up with two classes with static >> factory methods doing the same thing). > > Okay, that does simplify it a bit. > > Thanks, > Michael. > >> -Alan. >> >> >> >> >> >> Michael McMahon has updated the pull request incrementally with one additional commit since the last revision: unixdomainchannels: updates from Chris's review 9 Oct 2020 - Changes: - all: https://git.openjdk.java.net/jdk/pull/52/files - new: https://git.openjdk.java.net/jdk/pull/52/files/0e8e527f..cb28f3c0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=17 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=16-17 Stats: 18 lines in 2 files changed: 3 ins; 3 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/52.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/52/head:pull/52 PR: https://git.openjdk.java.net/jdk/pull/52
