On Wed, 24 Feb 2021 15:41:53 GMT, Daniel Fuchs wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Reduce the scope of the synchronized block in getInstance() and enhance
>> the testcase to be more robust in catching
On Fri, 19 Feb 2021 04:29:55 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` a
On Fri, 19 Feb 2021 05:15:24 GMT, Vyom Mani Tewari
wrote:
>> After thinking a bit more about this issue and the patch I had proposed, I
>> realized what I did wrong in this patch. The `synchronized` block in the
>> `sun.net.ext.ExtendedSocketOptions#getInstance()` was scoped one statement
>>
On Fri, 19 Feb 2021 04:26:26 GMT, Jaikiran Pai wrote:
> After thinking a bit more about this issue and the patch I had proposed, I
> realized what I did wrong in this patch. The `synchronized` block in the
> `sun.net.ext.ExtendedSocketOptions#getInstance()` was scoped one statement
> too many.
On Thu, 18 Feb 2021 13:06:44 GMT, Jaikiran Pai wrote:
>> Changes requested by dfuchs (Reviewer).
>
> I'm closing this for now, for the reason stated here
> https://github.com/openjdk/jdk/pull/2601#discussion_r578400669.
After thinking a bit more about this issue and the patch I had proposed, I
> 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.
On Thu, 18 Feb 2021 13:31:07 GMT, Jaikiran Pai wrote:
>> Hi Jaikiran,
>>
>> I tested with my suggested change and i did not see any deadlock at my local
>> Linux environment. I just ran test in loop and it worked as expected.
>>
>> Thanks,
>> Vyom
>
> Hello Vyom,
>
> The trick is to change t
On Thu, 18 Feb 2021 13:23:37 GMT, Vyom Mani Tewari
wrote:
>> Hello Daniel, I had thought about it in my previous commit. But this won't
>> work, since in the normal case where the `ClassNotFoundException` doesn't
>> get thrown, the `instance` is actually set in the `register` method which
>>
On Thu, 18 Feb 2021 13:05:48 GMT, Jaikiran Pai wrote:
>> src/java.base/share/classes/sun/net/ext/ExtendedSocketOptions.java line 189:
>>
>>> 187: }
>>> 188: }
>>> 189: return instance;
>>
>> I'd suggest changing these two lines as well:
>>
>> // the
On Thu, 18 Feb 2021 10:54:36 GMT, Daniel Fuchs wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> reduce volatile reads
>
> Changes requested by dfuchs (Reviewer).
I'm closing this for now, for the reason stated here
On Thu, 18 Feb 2021 10:44:03 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` a
On Thu, 18 Feb 2021 09:57:19 GMT, Jaikiran Pai wrote:
>> Let me be more clear: I think that it's enough to have only 2 `volatile`
>> field reads _in worst case_. We can use local variable to avoid more than
>> needed reads.
>> Current code can perform read 4 times _in worst case_: twice outside
> 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.
On Thu, 18 Feb 2021 07:59:45 GMT, Andrey Turbanov
wrote:
>> 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.
>
> Le
On Thu, 18 Feb 2021 05:01:41 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 redund
On Thu, 18 Feb 2021 07:35:51 GMT, Jaikiran Pai wrote:
>> 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 r
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
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 `
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 clas
> 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.
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 the
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
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
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` a
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
> 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.
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 severa
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 pote
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.
-
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` a
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 `j
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 beca
32 matches
Mail list logo
