Integrated: 8261750: Remove internal class sun.net.www.MimeLauncher
On Mon, 15 Feb 2021 18:30:58 GMT, Julia Boes wrote: > This change removes sun.net.www.MimeLauncher, a package-private class that is > no longer used. > > The sun.net.www package is exported to java.net.http and jdk.jartool. The > only public access point to a MimeLauncher instance is > sun.net.www.MimeEntry::launch, which is not called anywhere in those two > package. A related public class, sun.net.www. ApplicationLaunchException, is > no longer used anywhere. Therefore, these two can also be considered for > removal. This pull request has now been integrated. Changeset: 03b586b3 Author:Julia Boes URL: https://git.openjdk.java.net/jdk/commit/03b586b3 Stats: 301 lines in 3 files changed: 0 ins; 300 del; 1 mod 8261750: Remove internal class sun.net.www.MimeLauncher Reviewed-by: alanb, dfuchs - PR: https://git.openjdk.java.net/jdk/pull/2580
Re: RFR: JDK-8261791: handleSendFailed in SctpChannelImpl.c potential leaks
On Tue, 16 Feb 2021 12:26:54 GMT, Matthias Baesken wrote: > In another bug this question from me was answered by Alan Bateman : > > Btw. while adjusting Java_sun_nio_ch_sctp_SctpChannelImpl_receive0 , I > started to wonder what happens to the allocated memory in the same file in > handleSendFailed ( if ((addressP = malloc(dataLength)) == NULL) ) in early > return cases incl. the CHECK_NULL , is there some deallocation missing there > too ? > > > Yes, the error paths in handleSendFailed should be looked at. If > NewDirectByteBuffer or recvmsg fails then addressP needs to be freed. > Furthermore, if the NewObject fails and bufferObj != NULL then the memory for > the direct buffer will need to be freed too (as JNI NewDirectByteBuffer does > not setup a cleaner). > > > So I added freeing of the malloced memory to handleSendFailed . > Please review ! > > Thanks, Matthias Marked as reviewed by chegar (Reviewer). src/jdk.sctp/unix/native/libsctp/SctpChannelImpl.c line 236: > 234: return; > 235: } > 236: This looks fine. If NewDirectByteBuffer returns NULL there will be a pending OutOfMemoryError. src/jdk.sctp/unix/native/libsctp/SctpChannelImpl.c line 251: > 249: free(addressP); > 250: handleSocketError(env, errno); > 251: return; At this point the direct ByteBuffer has been successfully allocated and refers to the memory at addressP. But the direct ByteBuffer, while in the Java heap, will not have any references to it, so freeing addressP should be fine here. src/jdk.sctp/unix/native/libsctp/SctpChannelImpl.c line 257: > 255: //TODO: assert false: "should not reach here"; > 256: free(addressP); > 257: return; same a previous comment - looks ok. src/jdk.sctp/unix/native/libsctp/SctpChannelImpl.c line 270: > 268: return; > 269: } > 270: (*env)->SetObjectField(env, resultContainerObj, src_valueID, > resultObj); If NewObject returns NULL, there will be a pending OutOfMemoryError. Freeing addressP should be safe here, same reasoning as above. - PR: https://git.openjdk.java.net/jdk/pull/2586
Re: RFR: JDK-8261791: handleSendFailed in SctpChannelImpl.c potential leaks
On Wed, 17 Feb 2021 10:30:45 GMT, Chris Hegarty wrote: >> In another bug this question from me was answered by Alan Bateman : >> >> Btw. while adjusting Java_sun_nio_ch_sctp_SctpChannelImpl_receive0 , I >> started to wonder what happens to the allocated memory in the same file in >> handleSendFailed ( if ((addressP = malloc(dataLength)) == NULL) ) in early >> return cases incl. the CHECK_NULL , is there some deallocation missing there >> too ? >> >> >> Yes, the error paths in handleSendFailed should be looked at. If >> NewDirectByteBuffer or recvmsg fails then addressP needs to be freed. >> Furthermore, if the NewObject fails and bufferObj != NULL then the memory >> for the direct buffer will need to be freed too (as JNI NewDirectByteBuffer >> does not setup a cleaner). >> >> >> So I added freeing of the malloced memory to handleSendFailed . >> Please review ! >> >> Thanks, Matthias > > Marked as reviewed by chegar (Reviewer). This changes look good to me. Additionally, and separately, I have filed JDK-8261881. >From 8261881 : The SCTP implementation uses NewDirectByteBuffer to create a >new direct byte-buffer in native code. Such creations of a direct byte-buffer >are not automatically associated with a Cleaner, therefore the native memory >associated with the buffer may not be released when the buffer is no longer >reachable. The Java code that results in the native NewDirectByteBuffer should be examined with a view to creating a cleaner and assigning it to the direct byte-buffer. - PR: https://git.openjdk.java.net/jdk/pull/2586
Re: RFR: JDK-8261791: handleSendFailed in SctpChannelImpl.c potential leaks
On Tue, 16 Feb 2021 12:26:54 GMT, Matthias Baesken wrote: > In another bug this question from me was answered by Alan Bateman : > > Btw. while adjusting Java_sun_nio_ch_sctp_SctpChannelImpl_receive0 , I > started to wonder what happens to the allocated memory in the same file in > handleSendFailed ( if ((addressP = malloc(dataLength)) == NULL) ) in early > return cases incl. the CHECK_NULL , is there some deallocation missing there > too ? > > > Yes, the error paths in handleSendFailed should be looked at. If > NewDirectByteBuffer or recvmsg fails then addressP needs to be freed. > Furthermore, if the NewObject fails and bufferObj != NULL then the memory for > the direct buffer will need to be freed too (as JNI NewDirectByteBuffer does > not setup a cleaner). > > > So I added freeing of the malloced memory to handleSendFailed . > Please review ! > > Thanks, Matthias The changes looks okay to me. I see Chris has created JDK-8261881 to setup the cleanup. - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2586
Re: RFR: 8260366: ExtendedSocketOptions can deadlock in some circumstances
On Wed, 17 Feb 2021 07:20:27 GMT, Jaikiran Pai wrote:
> Can I please get a review for this change which proposes to fix the issue
> reported in https://bugs.openjdk.java.net/browse/JDK-8260366?
>
> The issue relates to the concurrent classloading of
> `sun.net.ext.ExtendedSocketOptions` and `jdk.net.ExtendedSocketOptions`
> leading to a deadlock. This is because the
> `sun.net.ext.ExtendedSocketOptions` in its static block does a
> `Class.forName("jdk.net.ExtendedSocketOptions")`. The
> `jdk.net.ExtendedSocketOptions` in its own static block calls the `register`
> method on `sun.net.ext.ExtendedSocketOptions`. If 2 threads concurrently try
> loading these classes (one loading the `sun.net.ext.ExtendedSocketOptions`
> and the other loading `jdk.net.ExtendedSocketOptions`), it can so happen that
> each one ends up holding a classloading lock in the static block of the
> respective class, while waiting for the other thread to release the lock,
> thus leading to a deadlock. The stacktrace posted in the linked JBS issue has
> the necessary details on the deadlock.
>
> The commit here breaks this deadlock by moving out the
> `Class.forName("jdk.net.ExtendedSocketOptions")` call from the static block
> of `sun.net.ext.ExtendedSocketOptions` to the `getInstance()` method, thus
> lazily loading (on first call to `getInstance()`) and registering the
> `jdk.net.ExtendedSocketOptions`.
>
> Extra attention needs to be given to the
> `sun.net.ext.ExtendedSocketOptions#register(ExtendedSocketOptions
> extOptions)` method. Before the change in this PR, when the
> `sun.net.ext.ExtendedSocketOptions` would successfully complete loading, it
> was guaranteed that the registered `ExtendedSocketOptions` would either be
> the one registered from the `jdk.net.ExtendedSocketOptions` or a
> `NoExtendedSocketOptions`. There wasn't any window of chance for any code (be
> it in the JDK or in application code) to call the
> `sun.net.ext.ExtendedSocketOptions#register` to register any different/other
> implementation/instance of the `ExtendedSocketOptions`. However, with this
> change in the PR, there is now a window of chance where any code in the JDK
> (or even application code?) can potentially call the
> `sun.net.ext.ExtendedSocketOptions#register` before either the
> `jdk.net.ExtendedSocketOptions` is loaded or the
> `sun.net.ext.ExtendedSocketOptions#getInstance()` method is called, thus
> allowing for some o
ther implementation of the `ExtendedSocketOptions` to be registered. However,
I'm not sure if it's a practical scenario - although the
`sun.net.ext.ExtendedSocketOptions#register` is marked `public`, the comment on
that method and the fact that it resides in an internal, not exposed by default
class/module, makes me believe that this `register` method isn't supposed to be
called by anyone other than the `jdk.net.ExtendedSocketOptions`. If at all this
`register` method is allowed to be called from other places, then due to the
change in this PR, additional work needs to be probably done in its
implementation to allow for the `jdk.net.ExtendedSocketOptions` to be given
first priority(?) to be registered first. I'll need input on whether I should
worry about this case or if it's fine in its current form in this PR.
>
> This PR also contains a jtreg test which reproduces the issue and verifies
> the fix.
Hi Jaikiran,
Thanks for taking a go at this one.
At a glance - the proposed fix seems reasonable to me. It would be good to have
another pair of eyes (and analysis) on this though. Some issues with the test.
test/jdk/sun/net/ext/ExtendedSocketOptionsTest.java line 39:
> 37: * @summary Verify that concurrent classloading of
> sun.net.ext.ExtendedSocketOptions and
> 38: * jdk.net.ExtendedSocketOptions doesn't lead to a deadlock
> 39: * @run testng/othervm --add-exports=java.base/sun.net.ext=ALL-UNNAMED
> ExtendedSocketOptionsTest
The --add-exports should not be needed - have you tried simply adding:
@modules java.base/sun.net.ext:+open
jdk.net
before `@run`? This also has the additional benefit to declare which modules
are required to run the test.
test/jdk/sun/net/ext/ExtendedSocketOptionsTest.java line 10:
> 8: * particular file as subject to the "Classpath" exception as provided
> 9: * by Oracle in the LICENSE file that accompanied this code.
> 10: *
Tests do not need - nor should they have - the "Classpath" exception - please
see Copyright notice of other tests in the vicinity.
-
Changes requested by dfuchs (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/2601
Re: RFR: 8260366: ExtendedSocketOptions can deadlock in some circumstances
On Wed, 17 Feb 2021 13:05:03 GMT, Daniel Fuchs wrote:
>> Can I please get a review for this change which proposes to fix the issue
>> reported in https://bugs.openjdk.java.net/browse/JDK-8260366?
>>
>> The issue relates to the concurrent classloading of
>> `sun.net.ext.ExtendedSocketOptions` and `jdk.net.ExtendedSocketOptions`
>> leading to a deadlock. This is because the
>> `sun.net.ext.ExtendedSocketOptions` in its static block does a
>> `Class.forName("jdk.net.ExtendedSocketOptions")`. The
>> `jdk.net.ExtendedSocketOptions` in its own static block calls the `register`
>> method on `sun.net.ext.ExtendedSocketOptions`. If 2 threads concurrently try
>> loading these classes (one loading the `sun.net.ext.ExtendedSocketOptions`
>> and the other loading `jdk.net.ExtendedSocketOptions`), it can so happen
>> that each one ends up holding a classloading lock in the static block of the
>> respective class, while waiting for the other thread to release the lock,
>> thus leading to a deadlock. The stacktrace posted in the linked JBS issue
>> has the necessary details on the deadlock.
>>
>> The commit here breaks this deadlock by moving out the
>> `Class.forName("jdk.net.ExtendedSocketOptions")` call from the static block
>> of `sun.net.ext.ExtendedSocketOptions` to the `getInstance()` method, thus
>> lazily loading (on first call to `getInstance()`) and registering the
>> `jdk.net.ExtendedSocketOptions`.
>>
>> Extra attention needs to be given to the
>> `sun.net.ext.ExtendedSocketOptions#register(ExtendedSocketOptions
>> extOptions)` method. Before the change in this PR, when the
>> `sun.net.ext.ExtendedSocketOptions` would successfully complete loading, it
>> was guaranteed that the registered `ExtendedSocketOptions` would either be
>> the one registered from the `jdk.net.ExtendedSocketOptions` or a
>> `NoExtendedSocketOptions`. There wasn't any window of chance for any code
>> (be it in the JDK or in application code) to call the
>> `sun.net.ext.ExtendedSocketOptions#register` to register any different/other
>> implementation/instance of the `ExtendedSocketOptions`. However, with this
>> change in the PR, there is now a window of chance where any code in the JDK
>> (or even application code?) can potentially call the
>> `sun.net.ext.ExtendedSocketOptions#register` before either the
>> `jdk.net.ExtendedSocketOptions` is loaded or the
>> `sun.net.ext.ExtendedSocketOptions#getInstance()` method is called, thus
>> allowing for some
other implementation of the `ExtendedSocketOptions` to be registered. However,
I'm not sure if it's a practical scenario - although the
`sun.net.ext.ExtendedSocketOptions#register` is marked `public`, the comment on
that method and the fact that it resides in an internal, not exposed by default
class/module, makes me believe that this `register` method isn't supposed to be
called by anyone other than the `jdk.net.ExtendedSocketOptions`. If at all this
`register` method is allowed to be called from other places, then due to the
change in this PR, additional work needs to be probably done in its
implementation to allow for the `jdk.net.ExtendedSocketOptions` to be given
first priority(?) to be registered first. I'll need input on whether I should
worry about this case or if it's fine in its current form in this PR.
>>
>> This PR also contains a jtreg test which reproduces the issue and verifies
>> the fix.
>
> test/jdk/sun/net/ext/ExtendedSocketOptionsTest.java line 39:
>
>> 37: * @summary Verify that concurrent classloading of
>> sun.net.ext.ExtendedSocketOptions and
>> 38: * jdk.net.ExtendedSocketOptions doesn't lead to a deadlock
>> 39: * @run testng/othervm --add-exports=java.base/sun.net.ext=ALL-UNNAMED
>> ExtendedSocketOptionsTest
>
> The --add-exports should not be needed - have you tried simply adding:
>
> @modules java.base/sun.net.ext:+open
> jdk.net
>
> before `@run`? This also has the additional benefit to declare which modules
> are required to run the test.
Since the test should be fast, I'd also advise to stick several identical @run
lines (maybe e.g. 5 of them) to increase the probability of the deadlock to
happen...
-
PR: https://git.openjdk.java.net/jdk/pull/2601
Re: RFR: JDK-8261791:(sctp) handleSendFailed in SctpChannelImpl.c potential leaks
On Wed, 17 Feb 2021 12:51:06 GMT, Alan Bateman wrote: >> In another bug this question from me was answered by Alan Bateman : >> >> Btw. while adjusting Java_sun_nio_ch_sctp_SctpChannelImpl_receive0 , I >> started to wonder what happens to the allocated memory in the same file in >> handleSendFailed ( if ((addressP = malloc(dataLength)) == NULL) ) in early >> return cases incl. the CHECK_NULL , is there some deallocation missing there >> too ? >> >> >> Yes, the error paths in handleSendFailed should be looked at. If >> NewDirectByteBuffer or recvmsg fails then addressP needs to be freed. >> Furthermore, if the NewObject fails and bufferObj != NULL then the memory >> for the direct buffer will need to be freed too (as JNI NewDirectByteBuffer >> does not setup a cleaner). >> >> >> So I added freeing of the malloced memory to handleSendFailed . >> Please review ! >> >> Thanks, Matthias > > The changes looks okay to me. I see Chris has created JDK-8261881 to setup > the cleanup. Hi Alan and Chris, thanks for the reviews. - PR: https://git.openjdk.java.net/jdk/pull/2586
Re: RFR: JDK-8261791:(sctp) handleSendFailed in SctpChannelImpl.c potential leaks
On Wed, 17 Feb 2021 13:19:13 GMT, Matthias Baesken wrote: >> The changes looks okay to me. I see Chris has created JDK-8261881 to setup >> the cleanup. > > Hi Alan and Chris, thanks for the reviews. Looks like I cannot integrate it, even after changing the title!? Is this a bug? - PR: https://git.openjdk.java.net/jdk/pull/2586
Re: RFR: JDK-8261791:(sctp) handleSendFailed in SctpChannelImpl.c potential leaks
On Wed, 17 Feb 2021 13:27:13 GMT, Matthias Baesken wrote: >> Hi Alan and Chris, thanks for the reviews. > > Looks like I cannot integrate it, even after changing the title!? > Is this a bug? Here's the jcheck failure: https://github.com/openjdk/jdk/pull/2586/checks?check_run_id=1918880270 - PR: https://git.openjdk.java.net/jdk/pull/2586
Re: RFR: 8260366: ExtendedSocketOptions can deadlock in some circumstances [v2]
On Wed, 17 Feb 2021 13:09:27 GMT, Daniel Fuchs wrote: > Since the test should be fast, I'd also advise to stick several identical > @run lines (maybe e.g. 5 of them) to increase the probability of the deadlock > to happen... Done. The PR has been updated to run this test multiple times now. - PR: https://git.openjdk.java.net/jdk/pull/2601
Re: RFR: 8260366: ExtendedSocketOptions can deadlock in some circumstances [v2]
On Wed, 17 Feb 2021 13:11:23 GMT, Daniel Fuchs wrote: >> Jaikiran Pai has updated the pull request incrementally with three >> additional commits since the last revision: >> >> - Incorporate the review suggestion to run the test multiple times to >> improve the chances of reproducing any potential deadlock >> - Incorporate the review suggestion to use @modules instead of >> --add-exports option while launching the test >> - Fix copyright message on test > > Hi Jaikiran, > > Thanks for taking a go at this one. > > At a glance - the proposed fix seems reasonable to me. It would be good to > have another pair of eyes (and analysis) on this though. Some issues with the > test. Hello Daniel, thank you for the review. I have updated this PR to incorporate your review suggestions for the test. > test/jdk/sun/net/ext/ExtendedSocketOptionsTest.java line 10: > >> 8: * particular file as subject to the "Classpath" exception as provided >> 9: * by Oracle in the LICENSE file that accompanied this code. >> 10: * > > Tests do not need - nor should they have - the "Classpath" exception - please > see Copyright notice of other tests in the vicinity. Thank you for catching this. I've fixed it in the latest update of the PR. - PR: https://git.openjdk.java.net/jdk/pull/2601
Re: RFR: 8260366: ExtendedSocketOptions can deadlock in some circumstances [v2]
On Wed, 17 Feb 2021 13:37:19 GMT, Jaikiran Pai wrote: >> Since the test should be fast, I'd also advise to stick several identical >> @run lines (maybe e.g. 5 of them) to increase the probability of the >> deadlock to happen... > >> Since the test should be fast, I'd also advise to stick several identical >> @run lines (maybe e.g. 5 of them) to increase the probability of the >> deadlock to happen... > > Done. The PR has been updated to run this test multiple times now. > The --add-exports should not be needed - have you tried simply adding: > > ``` > @modules java.base/sun.net.ext:+open > jdk.net > ``` > > before `@run`? This also has the additional benefit to declare which modules > are required to run the test. I had forgotten about the `@modules` directive. I have now updated the PR use that instead. The only minor difference between what you suggested and my updated PR is that I decided to use `:open` instead of `:+open` for the `sun.net.ext` package, since I don't use the types in that package at compile time, in that test. - PR: https://git.openjdk.java.net/jdk/pull/2601
Re: RFR: 8260366: ExtendedSocketOptions can deadlock in some circumstances [v2]
> Can I please get a review for this change which proposes to fix the issue
> reported in https://bugs.openjdk.java.net/browse/JDK-8260366?
>
> The issue relates to the concurrent classloading of
> `sun.net.ext.ExtendedSocketOptions` and `jdk.net.ExtendedSocketOptions`
> leading to a deadlock. This is because the
> `sun.net.ext.ExtendedSocketOptions` in its static block does a
> `Class.forName("jdk.net.ExtendedSocketOptions")`. The
> `jdk.net.ExtendedSocketOptions` in its own static block calls the `register`
> method on `sun.net.ext.ExtendedSocketOptions`. If 2 threads concurrently try
> loading these classes (one loading the `sun.net.ext.ExtendedSocketOptions`
> and the other loading `jdk.net.ExtendedSocketOptions`), it can so happen that
> each one ends up holding a classloading lock in the static block of the
> respective class, while waiting for the other thread to release the lock,
> thus leading to a deadlock. The stacktrace posted in the linked JBS issue has
> the necessary details on the deadlock.
>
> The commit here breaks this deadlock by moving out the
> `Class.forName("jdk.net.ExtendedSocketOptions")` call from the static block
> of `sun.net.ext.ExtendedSocketOptions` to the `getInstance()` method, thus
> lazily loading (on first call to `getInstance()`) and registering the
> `jdk.net.ExtendedSocketOptions`.
>
> Extra attention needs to be given to the
> `sun.net.ext.ExtendedSocketOptions#register(ExtendedSocketOptions
> extOptions)` method. Before the change in this PR, when the
> `sun.net.ext.ExtendedSocketOptions` would successfully complete loading, it
> was guaranteed that the registered `ExtendedSocketOptions` would either be
> the one registered from the `jdk.net.ExtendedSocketOptions` or a
> `NoExtendedSocketOptions`. There wasn't any window of chance for any code (be
> it in the JDK or in application code) to call the
> `sun.net.ext.ExtendedSocketOptions#register` to register any different/other
> implementation/instance of the `ExtendedSocketOptions`. However, with this
> change in the PR, there is now a window of chance where any code in the JDK
> (or even application code?) can potentially call the
> `sun.net.ext.ExtendedSocketOptions#register` before either the
> `jdk.net.ExtendedSocketOptions` is loaded or the
> `sun.net.ext.ExtendedSocketOptions#getInstance()` method is called, thus
> allowing for some o
ther implementation of the `ExtendedSocketOptions` to be registered. However,
I'm not sure if it's a practical scenario - although the
`sun.net.ext.ExtendedSocketOptions#register` is marked `public`, the comment on
that method and the fact that it resides in an internal, not exposed by default
class/module, makes me believe that this `register` method isn't supposed to be
called by anyone other than the `jdk.net.ExtendedSocketOptions`. If at all this
`register` method is allowed to be called from other places, then due to the
change in this PR, additional work needs to be probably done in its
implementation to allow for the `jdk.net.ExtendedSocketOptions` to be given
first priority(?) to be registered first. I'll need input on whether I should
worry about this case or if it's fine in its current form in this PR.
>
> This PR also contains a jtreg test which reproduces the issue and verifies
> the fix.
Jaikiran Pai has updated the pull request incrementally with three additional
commits since the last revision:
- Incorporate the review suggestion to run the test multiple times to improve
the chances of reproducing any potential deadlock
- Incorporate the review suggestion to use @modules instead of --add-exports
option while launching the test
- Fix copyright message on test
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/2601/files
- new: https://git.openjdk.java.net/jdk/pull/2601/files/7aaed261..62cf35cd
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2601&range=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2601&range=00-01
Stats: 10 lines in 1 file changed: 6 ins; 2 del; 2 mod
Patch: https://git.openjdk.java.net/jdk/pull/2601.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2601/head:pull/2601
PR: https://git.openjdk.java.net/jdk/pull/2601
Re: RFR: JDK-8261791:(sctp) handleSendFailed in SctpChannelImpl.c potential leaks
On Wed, 17 Feb 2021 13:36:17 GMT, Alan Bateman wrote: >> Looks like I cannot integrate it, even after changing the title!? >> Is this a bug? > > Here's the jcheck failure: > https://github.com/openjdk/jdk/pull/2586/checks?check_run_id=1918880270 As pointed out by Alan: It seems like a space is missing after the colon in the PR title - PR: https://git.openjdk.java.net/jdk/pull/2586
Re: RFR: JDK-8261791: (sctp) handleSendFailed in SctpChannelImpl.c potential leaks
On Wed, 17 Feb 2021 13:36:17 GMT, Alan Bateman wrote: > Here's the jcheck failure: > https://github.com/openjdk/jdk/pull/2586/checks?check_run_id=1918880270 Hi Alan , I am aware of the jcheck message. It claims : "The commit message does not reference any issue. " I adjusted now the for the 3 or 4th time the message "JDK-8261791:(sctp) handleSendFailed in SctpChannelImpl.c potential leaks" without any progress . I think this is because it was modified in JBS after I created it (never had this problem without such a JBS modification). - PR: https://git.openjdk.java.net/jdk/pull/2586
Integrated: JDK-8261791: (sctp) handleSendFailed in SctpChannelImpl.c potential leaks
On Tue, 16 Feb 2021 12:26:54 GMT, Matthias Baesken wrote: > In another bug this question from me was answered by Alan Bateman : > > Btw. while adjusting Java_sun_nio_ch_sctp_SctpChannelImpl_receive0 , I > started to wonder what happens to the allocated memory in the same file in > handleSendFailed ( if ((addressP = malloc(dataLength)) == NULL) ) in early > return cases incl. the CHECK_NULL , is there some deallocation missing there > too ? > > > Yes, the error paths in handleSendFailed should be looked at. If > NewDirectByteBuffer or recvmsg fails then addressP needs to be freed. > Furthermore, if the NewObject fails and bufferObj != NULL then the memory for > the direct buffer will need to be freed too (as JNI NewDirectByteBuffer does > not setup a cleaner). > > > So I added freeing of the malloced memory to handleSendFailed . > Please review ! > > Thanks, Matthias This pull request has now been integrated. Changeset: a0658795 Author:Matthias Baesken URL: https://git.openjdk.java.net/jdk/commit/a0658795 Stats: 10 lines in 1 file changed: 8 ins; 0 del; 2 mod 8261791: (sctp) handleSendFailed in SctpChannelImpl.c potential leaks Reviewed-by: chegar, alanb - PR: https://git.openjdk.java.net/jdk/pull/2586
Re: RFR: JDK-8261791: (sctp) handleSendFailed in SctpChannelImpl.c potential leaks
On Wed, 17 Feb 2021 13:43:14 GMT, Daniel Fuchs wrote: > As pointed out by Alan: It seems like a space is missing after the colon in > the PR title lets give it a try. Thanks ! - PR: https://git.openjdk.java.net/jdk/pull/2586
RFR: 8261880: Change nested classes in java.base to static nested classes where possible
Non-static classes hold a link to their parent classes, which in many cases can be avoided. - Commit messages: - 8261880: Change nested classes in java.base to static nested classes where possible Changes: https://git.openjdk.java.net/jdk/pull/2589/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2589&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261880 Stats: 20 lines in 16 files changed: 0 ins; 0 del; 20 mod Patch: https://git.openjdk.java.net/jdk/pull/2589.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2589/head:pull/2589 PR: https://git.openjdk.java.net/jdk/pull/2589
Re: RFR: 8261880: Change nested classes in java.base to static nested classes where possible
On Tue, 16 Feb 2021 14:30:58 GMT, Сергей Цыпанов
wrote:
> Non-static classes hold a link to their parent classes, which in many cases
> can be avoided.
src/java.base/share/classes/java/lang/invoke/DelegatingMethodHandle.java line
192:
> 190:
> 191: /* Placeholder class for DelegatingMethodHandles generated ahead of
> time */
> 192: static final class Holder {}
For the four `Holder` classes in `java.lang.invoke`, the class is generated
when running jlink via `java.lang.invoke.GenerateJLIClassesHelper`. To stay in
sync with the definition you'd have to update that code. Also, since these
`Holder` classes will only contain static methods and are never instantiated
then I'm not sure it matters whether the classes are static or not.
-
PR: https://git.openjdk.java.net/jdk/pull/2589
Re: RFR: 8261880: Change nested classes in java.base to static nested classes where possible [v2]
On Wed, 17 Feb 2021 16:24:46 GMT, Claes Redestad wrote:
>> Сергей Цыпанов has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8261880: Remove static from declarations of Holder nested classes
>
> src/java.base/share/classes/java/lang/invoke/DelegatingMethodHandle.java line
> 192:
>
>> 190:
>> 191: /* Placeholder class for DelegatingMethodHandles generated ahead of
>> time */
>> 192: static final class Holder {}
>
> For the four `Holder` classes in `java.lang.invoke`, the class is generated
> when running jlink via `java.lang.invoke.GenerateJLIClassesHelper`. To stay
> in sync with the definition you'd have to update that code. Also, since these
> `Holder` classes will only contain static methods and are never instantiated
> then I'm not sure it matters whether the classes are static or not.
I'll just revert them
-
PR: https://git.openjdk.java.net/jdk/pull/2589
Re: RFR: 8261880: Change nested classes in java.base to static nested classes where possible [v2]
> Non-static classes hold a link to their parent classes, which in many cases > can be avoided. Сергей Цыпанов has updated the pull request incrementally with one additional commit since the last revision: 8261880: Remove static from declarations of Holder nested classes - Changes: - all: https://git.openjdk.java.net/jdk/pull/2589/files - new: https://git.openjdk.java.net/jdk/pull/2589/files/5650cce4..c6f9cf6b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2589&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2589&range=00-01 Stats: 4 lines in 4 files changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/2589.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2589/head:pull/2589 PR: https://git.openjdk.java.net/jdk/pull/2589
Re: RFR: 8261880: Change nested classes in java.base to static nested classes where possible [v2]
On Wed, 17 Feb 2021 16:32:39 GMT, Сергей Цыпанов
wrote:
>> src/java.base/share/classes/java/lang/invoke/DelegatingMethodHandle.java
>> line 192:
>>
>>> 190:
>>> 191: /* Placeholder class for DelegatingMethodHandles generated ahead
>>> of time */
>>> 192: static final class Holder {}
>>
>> For the four `Holder` classes in `java.lang.invoke`, the class is generated
>> when running jlink via `java.lang.invoke.GenerateJLIClassesHelper`. To stay
>> in sync with the definition you'd have to update that code. Also, since
>> these `Holder` classes will only contain static methods and are never
>> instantiated then I'm not sure it matters whether the classes are static or
>> not.
>
> I'll just revert them
For static methods, since in java language you cannot declare static method in
instance inner classes, I'd say making them static makes more sense
language-wise. Also making them static reduces compiler synthetic instance
field and constructors.
-
PR: https://git.openjdk.java.net/jdk/pull/2589
Re: RFR: 8260366: ExtendedSocketOptions can deadlock in some circumstances [v2]
On Wed, 17 Feb 2021 13:36:18 GMT, Jaikiran Pai wrote: >> Hi Jaikiran, >> >> Thanks for taking a go at this one. >> >> At a glance - the proposed fix seems reasonable to me. It would be good to >> have another pair of eyes (and analysis) on this though. Some issues with >> the test. > > Hello Daniel, thank you for the review. I have updated this PR to incorporate > your review suggestions for the test. Changes looks OK to me, any specific reason for removing "final" specifier from "getInstance" & "register" methods ?. - PR: https://git.openjdk.java.net/jdk/pull/2601
Re: RFR: 8261880: Change nested classes in java.base to static nested classes where possible [v2]
On Wed, 17 Feb 2021 16:35:02 GMT, liach wrote: >> I'll just revert them > > For static methods, since in java language you cannot declare static method > in instance inner classes, I'd say making them static makes more sense > language-wise. Also making them static reduces compiler synthetic instance > field and constructors. Incidentally, Java-the-language allows static methods in inner instance classes since JDK 16. And I'm not sure this was ever a restriction at the JVMS level since we've been generating static methods (using ASM) into these inner instance classes since at least JDK 9. - PR: https://git.openjdk.java.net/jdk/pull/2589
Re: RFR: 8261880: Change nested classes in java.base to static nested classes where possible [v2]
On Wed, 17 Feb 2021 17:24:50 GMT, Claes Redestad wrote: >> For static methods, since in java language you cannot declare static method >> in instance inner classes, I'd say making them static makes more sense >> language-wise. Also making them static reduces compiler synthetic instance >> field and constructors. > > Incidentally, Java-the-language allows static methods in inner instance > classes since JDK 16. And I'm not sure this was ever a restriction at the > JVMS level since we've been generating static methods (using ASM) into these > inner instance classes since at least JDK 9. Inner classes doesn't really exist for the JVM, it's just an attribute (in fact, a pair of attributes) that is read/write by javac (it's very similar to the way generics work). So it is Ok to have static methods in inner classes since Java 1.1 from the JVM POV with the caveat that you may not be all to call them from Java-the-language. Also since Java 11, inner classes are also nestmate and those attributes (NestHost/NestMembers) change the behavior of the VM. - PR: https://git.openjdk.java.net/jdk/pull/2589
Re: RFR: 8260366: ExtendedSocketOptions can deadlock in some circumstances [v2]
On Wed, 17 Feb 2021 13:42:57 GMT, Jaikiran Pai wrote:
>> Can I please get a review for this change which proposes to fix the issue
>> reported in https://bugs.openjdk.java.net/browse/JDK-8260366?
>>
>> The issue relates to the concurrent classloading of
>> `sun.net.ext.ExtendedSocketOptions` and `jdk.net.ExtendedSocketOptions`
>> leading to a deadlock. This is because the
>> `sun.net.ext.ExtendedSocketOptions` in its static block does a
>> `Class.forName("jdk.net.ExtendedSocketOptions")`. The
>> `jdk.net.ExtendedSocketOptions` in its own static block calls the `register`
>> method on `sun.net.ext.ExtendedSocketOptions`. If 2 threads concurrently try
>> loading these classes (one loading the `sun.net.ext.ExtendedSocketOptions`
>> and the other loading `jdk.net.ExtendedSocketOptions`), it can so happen
>> that each one ends up holding a classloading lock in the static block of the
>> respective class, while waiting for the other thread to release the lock,
>> thus leading to a deadlock. The stacktrace posted in the linked JBS issue
>> has the necessary details on the deadlock.
>>
>> The commit here breaks this deadlock by moving out the
>> `Class.forName("jdk.net.ExtendedSocketOptions")` call from the static block
>> of `sun.net.ext.ExtendedSocketOptions` to the `getInstance()` method, thus
>> lazily loading (on first call to `getInstance()`) and registering the
>> `jdk.net.ExtendedSocketOptions`.
>>
>> Extra attention needs to be given to the
>> `sun.net.ext.ExtendedSocketOptions#register(ExtendedSocketOptions
>> extOptions)` method. Before the change in this PR, when the
>> `sun.net.ext.ExtendedSocketOptions` would successfully complete loading, it
>> was guaranteed that the registered `ExtendedSocketOptions` would either be
>> the one registered from the `jdk.net.ExtendedSocketOptions` or a
>> `NoExtendedSocketOptions`. There wasn't any window of chance for any code
>> (be it in the JDK or in application code) to call the
>> `sun.net.ext.ExtendedSocketOptions#register` to register any different/other
>> implementation/instance of the `ExtendedSocketOptions`. However, with this
>> change in the PR, there is now a window of chance where any code in the JDK
>> (or even application code?) can potentially call the
>> `sun.net.ext.ExtendedSocketOptions#register` before either the
>> `jdk.net.ExtendedSocketOptions` is loaded or the
>> `sun.net.ext.ExtendedSocketOptions#getInstance()` method is called, thus
>> allowing for some
other implementation of the `ExtendedSocketOptions` to be registered. However,
I'm not sure if it's a practical scenario - although the
`sun.net.ext.ExtendedSocketOptions#register` is marked `public`, the comment on
that method and the fact that it resides in an internal, not exposed by default
class/module, makes me believe that this `register` method isn't supposed to be
called by anyone other than the `jdk.net.ExtendedSocketOptions`. If at all this
`register` method is allowed to be called from other places, then due to the
change in this PR, additional work needs to be probably done in its
implementation to allow for the `jdk.net.ExtendedSocketOptions` to be given
first priority(?) to be registered first. I'll need input on whether I should
worry about this case or if it's fine in its current form in this PR.
>>
>> This PR also contains a jtreg test which reproduces the issue and verifies
>> the fix.
>
> Jaikiran Pai has updated the pull request incrementally with three additional
> commits since the last revision:
>
> - Incorporate the review suggestion to run the test multiple times to
> improve the chances of reproducing any potential deadlock
> - Incorporate the review suggestion to use @modules instead of --add-exports
> option while launching the test
> - Fix copyright message on test
src/java.base/share/classes/sun/net/ext/ExtendedSocketOptions.java line 171:
> 169:
> 170: public static ExtendedSocketOptions getInstance() {
> 171: if (instance != null) {
May be it's worth to avoid reading `volatile` field twice?
-
PR: https://git.openjdk.java.net/jdk/pull/2601
Re: RFR: 8260366: ExtendedSocketOptions can deadlock in some circumstances [v2]
On Wed, 17 Feb 2021 16:47:51 GMT, Vyom Mani Tewari wrote: > Changes looks OK to me, any specific reason for removing "final" specifier > from "getInstance" & "register" methods ?. Hello Vyom, thank you for the review. Since those two methods are `static`, the `final` was redundant on them and since this patch was already changing those 2 methods, I decided to remove it while I was at it. However, if you and others feel that this patch shouldn't change it, I will introduce it back. - PR: https://git.openjdk.java.net/jdk/pull/2601
Re: RFR: 8260366: ExtendedSocketOptions can deadlock in some circumstances [v2]
On Wed, 17 Feb 2021 20:01:17 GMT, Andrey Turbanov
wrote:
>> Jaikiran Pai has updated the pull request incrementally with three
>> additional commits since the last revision:
>>
>> - Incorporate the review suggestion to run the test multiple times to
>> improve the chances of reproducing any potential deadlock
>> - Incorporate the review suggestion to use @modules instead of
>> --add-exports option while launching the test
>> - Fix copyright message on test
>
> src/java.base/share/classes/sun/net/ext/ExtendedSocketOptions.java line 171:
>
>> 169:
>> 170: public static ExtendedSocketOptions getInstance() {
>> 171: if (instance != null) {
>
> May be it's worth to avoid reading `volatile` field twice?
Hello @turbanoff, do you mean why read it twice - once here and once inside the
`synchronized` block?
-
PR: https://git.openjdk.java.net/jdk/pull/2601
Re: RFR: 8260366: ExtendedSocketOptions can deadlock in some circumstances [v2]
On Thu, 18 Feb 2021 01:06:50 GMT, Jaikiran Pai wrote:
> > Changes looks OK to me, any specific reason for removing "final" specifier
> > from "getInstance" & "register" methods ?.
>
> Hello Vyom, thank you for the review. Since those two methods are `static`,
> the `final` was redundant on them and since this patch was already changing
> those 2 methods, I decided to remove it while I was at it. However, if you
> and others feel that this patch shouldn't change it, I will introduce it back.
I think it's OK for me. What about improving the test little bit, your test
wants to load both classes at the same time. Please have a look on modified
test.
/*
* Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/
import org.testng.Assert;
import org.testng.annotations.Test;
import java.util.concurrent.Callable;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
/**
* @test
* @bug 8260366
* @summary Verify that concurrent classloading of
* sun.net.ext.ExtendedSocketOptions and jdk.net.ExtendedSocketOptions doesn't
* lead to a deadlock
* @modules java.base/sun.net.ext:open
* @run testng/othervm ExtendedSocketOptionsTest
* @run testng/othervm ExtendedSocketOptionsTest
* @run testng/othervm ExtendedSocketOptionsTest
* @run testng/othervm ExtendedSocketOptionsTest
* @run testng/othervm ExtendedSocketOptionsTest
*/
public class ExtendedSocketOptionsTest {
/**
* Loads {@code jdk.net.ExtendedSocketOptions} and
* {@code sun.net.ext.ExtendedSocketOptions} concurrently in a thread of
* their own and expects the classloading of both those classes to
* succeed.Additionally, after the classloading is successfully done, calls
* the sun.net.ext.ExtendedSocketOptions#getInstance() and expects it to
* return a registered ExtendedSocketOptions instance.
*
* @throws java.lang.Exception
*/
@Test
public void testConcurrentClassLoad() throws Exception {
CountDownLatch latch = new CountDownLatch(1);
final Callable> task1 = new
Task("jdk.net.ExtendedSocketOptions", latch);
final Callable> task2 = new
Task("sun.net.ext.ExtendedSocketOptions", latch);
final ExecutorService executor = Executors.newFixedThreadPool(2);
try {
final Future>[] results = new Future[2];
// submit
results[0] = executor.submit(task1);
results[1] = executor.submit(task2);
latch.countDown();
// wait for completion
for (Future> result: results) {
final Class k = result.get();
System.out.println("Completed loading " + k.getName());
}
} finally {
executor.shutdownNow();
}
// check that the sun.net.ext.ExtendedSocketOptions#getInstance() does
indeed return
// the registered instance
final Class k = Class.forName("sun.net.ext.ExtendedSocketOptions");
final Object extSocketOptions =
k.getDeclaredMethod("getInstance").invoke(null);
Assert.assertNotNull(extSocketOptions,
"sun.net.ext.ExtendedSocketOptions#getInstance()"
+ " unexpectedly returned null");
}
private static class Task implements Callable> {
private final String className;
private final CountDownLatch latch;
private Task(final String className, CountDownLatch latch) {
this.className = className;
this.latch = latch;
}
@Override
public Class call() {
System.out.println(Thread.currentThread().getName() + " loading " +
this.className);
try {
latch.await();
return Class.forName(this.className);
} catch (ClassNotFoundException | InterruptedException e) {
System.err.println("Failed to load " + this.className);
throw new
Re: RFR: 8260366: ExtendedSocketOptions can deadlock in some circumstances [v3]
> Can I please get a review for this change which proposes to fix the issue
> reported in https://bugs.openjdk.java.net/browse/JDK-8260366?
>
> The issue relates to the concurrent classloading of
> `sun.net.ext.ExtendedSocketOptions` and `jdk.net.ExtendedSocketOptions`
> leading to a deadlock. This is because the
> `sun.net.ext.ExtendedSocketOptions` in its static block does a
> `Class.forName("jdk.net.ExtendedSocketOptions")`. The
> `jdk.net.ExtendedSocketOptions` in its own static block calls the `register`
> method on `sun.net.ext.ExtendedSocketOptions`. If 2 threads concurrently try
> loading these classes (one loading the `sun.net.ext.ExtendedSocketOptions`
> and the other loading `jdk.net.ExtendedSocketOptions`), it can so happen that
> each one ends up holding a classloading lock in the static block of the
> respective class, while waiting for the other thread to release the lock,
> thus leading to a deadlock. The stacktrace posted in the linked JBS issue has
> the necessary details on the deadlock.
>
> The commit here breaks this deadlock by moving out the
> `Class.forName("jdk.net.ExtendedSocketOptions")` call from the static block
> of `sun.net.ext.ExtendedSocketOptions` to the `getInstance()` method, thus
> lazily loading (on first call to `getInstance()`) and registering the
> `jdk.net.ExtendedSocketOptions`.
>
> Extra attention needs to be given to the
> `sun.net.ext.ExtendedSocketOptions#register(ExtendedSocketOptions
> extOptions)` method. Before the change in this PR, when the
> `sun.net.ext.ExtendedSocketOptions` would successfully complete loading, it
> was guaranteed that the registered `ExtendedSocketOptions` would either be
> the one registered from the `jdk.net.ExtendedSocketOptions` or a
> `NoExtendedSocketOptions`. There wasn't any window of chance for any code (be
> it in the JDK or in application code) to call the
> `sun.net.ext.ExtendedSocketOptions#register` to register any different/other
> implementation/instance of the `ExtendedSocketOptions`. However, with this
> change in the PR, there is now a window of chance where any code in the JDK
> (or even application code?) can potentially call the
> `sun.net.ext.ExtendedSocketOptions#register` before either the
> `jdk.net.ExtendedSocketOptions` is loaded or the
> `sun.net.ext.ExtendedSocketOptions#getInstance()` method is called, thus
> allowing for some o
ther implementation of the `ExtendedSocketOptions` to be registered. However,
I'm not sure if it's a practical scenario - although the
`sun.net.ext.ExtendedSocketOptions#register` is marked `public`, the comment on
that method and the fact that it resides in an internal, not exposed by default
class/module, makes me believe that this `register` method isn't supposed to be
called by anyone other than the `jdk.net.ExtendedSocketOptions`. If at all this
`register` method is allowed to be called from other places, then due to the
change in this PR, additional work needs to be probably done in its
implementation to allow for the `jdk.net.ExtendedSocketOptions` to be given
first priority(?) to be registered first. I'll need input on whether I should
worry about this case or if it's fine in its current form in this PR.
>
> This PR also contains a jtreg test which reproduces the issue and verifies
> the fix.
Jaikiran Pai has updated the pull request incrementally with one additional
commit since the last revision:
Use a latch in the testcase to trigger the classloading in multiple threads
as concurrently as possible
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/2601/files
- new: https://git.openjdk.java.net/jdk/pull/2601/files/62cf35cd..f1c551ab
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2601&range=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2601&range=01-02
Stats: 11 lines in 1 file changed: 8 ins; 0 del; 3 mod
Patch: https://git.openjdk.java.net/jdk/pull/2601.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2601/head:pull/2601
PR: https://git.openjdk.java.net/jdk/pull/2601
Re: RFR: 8260366: ExtendedSocketOptions can deadlock in some circumstances [v3]
On Thu, 18 Feb 2021 04:31:56 GMT, Vyom Mani Tewari wrote: > What about improving the test little bit, your test wants to load both > classes at the same time. Please have a look on modified test. Hello Vyom, I think that's a good suggestion to use a latch for deciding when to trigger the classloading. I've taken your input and have made some relatively minor change to the way that latch gets used and updated my PR with that change. The latch now waits for both the tasks to reach the point where they are going to do a `Class.forName` on their respectively class names. This should make the test trigger that classloading in separate threads as simultaneously as possible. - PR: https://git.openjdk.java.net/jdk/pull/2601
Re: RFR: 8260366: ExtendedSocketOptions can deadlock in some circumstances [v2]
On Thu, 18 Feb 2021 01:08:30 GMT, Jaikiran Pai wrote:
>> src/java.base/share/classes/sun/net/ext/ExtendedSocketOptions.java line 171:
>>
>>> 169:
>>> 170: public static ExtendedSocketOptions getInstance() {
>>> 171: if (instance != null) {
>>
>> May be it's worth to avoid reading `volatile` field twice?
>
> Hello @turbanoff, do you mean why read it twice - once here and once inside
> the `synchronized` block?
Yes. Once here and once inside `synchronized` block.
Reading `volatile` fields cost _something_ on some architectures, so I think we
could optimize it a bit.
-
PR: https://git.openjdk.java.net/jdk/pull/2601
Re: RFR: 8080272 Refactor I/O stream copying to use InputStream.transferTo/readAllBytes and Files.copy
On Tue, 16 Feb 2021 17:20:37 GMT, Julia Boes wrote: >> 8080272 Refactor I/O stream copying to use >> InputStream.transferTo/readAllBytes and Files.copy > > Hi @turbanoff, I'm happy to sponsor but I see two comments by @marschall - > have they been addressed? > https://github.com/openjdk/jdk/pull/1853#discussion_r572815422 > https://github.com/openjdk/jdk/pull/1853#discussion_r572380746 @FrauBoes fixed in the last commit. Is there any way to de-_integrate_ PR (to include last commit)? - PR: https://git.openjdk.java.net/jdk/pull/1853
Re: RFR: 8260366: ExtendedSocketOptions can deadlock in some circumstances [v2]
On Thu, 18 Feb 2021 07:03:07 GMT, Andrey Turbanov wrote: >> Hello @turbanoff, do you mean why read it twice - once here and once inside >> the `synchronized` block? > > Yes. Once here and once inside `synchronized` block. > Reading `volatile` fields cost _something_ on some architectures, so I think > we could optimize it a bit. Hello @turbanoff, the double read is necessary to correctly avoid any race conditions and is a typical strategy used in cases like these. I am not aware of any other alternate more performant strategy, for code like this. - PR: https://git.openjdk.java.net/jdk/pull/2601
