<Swing Dev> [10] Review Request: 5031664 Increase thread safety of EventListenerList

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

<Swing Dev> [10] Review Request: 5031664 Increase thread safety of EventListenerList

Sergey Bylokhov
Hello,
Please review the fix for jdk10.

The EventListenerList class was implemented to be thread safe. To
achieve the correct state of the object:
  - two mutators(add/remove) were marked as synchronized.
  - the internal array is updated via copy-on-write, the internal array
is never modified after it was set.

It was assumed that the getXXX methods will get the value which was
already set by the mutator or the previous value if mutator is
in-progress but was not update the array. The problem is that the getXXX
is not necessary read the value which was set, because of lack of
happens-before between add/remove and getXXX. In the fix the array was
marked as volatile to solve the problem.

The CSR will be created after the technical review (the field is
protected in public class)

Bug: https://bugs.openjdk.java.net/browse/JDK-5031664
Webrev can be found at: http://cr.openjdk.java.net/~serb/5031664/webrev.00


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10] Review Request: 5031664 Increase thread safety of EventListenerList

Alexander Zvegintsev
looks fine to me.

Thanks,

Alexander.

On 10/27/2017 04:48 AM, Sergey Bylokhov wrote:

> Hello,
> Please review the fix for jdk10.
>
> The EventListenerList class was implemented to be thread safe. To
> achieve the correct state of the object:
>  - two mutators(add/remove) were marked as synchronized.
>  - the internal array is updated via copy-on-write, the internal array
> is never modified after it was set.
>
> It was assumed that the getXXX methods will get the value which was
> already set by the mutator or the previous value if mutator is
> in-progress but was not update the array. The problem is that the
> getXXX is not necessary read the value which was set, because of lack
> of happens-before between add/remove and getXXX. In the fix the array
> was marked as volatile to solve the problem.
>
> The CSR will be created after the technical review (the field is
> protected in public class)
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-5031664
> Webrev can be found at:
> http://cr.openjdk.java.net/~serb/5031664/webrev.00
>
>

Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10] Review Request: 5031664 Increase thread safety of EventListenerList

Andrej Golovnin-2
In reply to this post by Sergey Bylokhov
Hi Sergey,

the order of modifiers is wrong. It should be "protected transient
volatile". See http://cr.openjdk.java.net/~alundblad/styleguide/index-v6.html#toc-modifiers
for details.

Best regards,
Andrej Golovnin

On Fri, Oct 27, 2017 at 1:18 AM, Sergey Bylokhov
<[hidden email]> wrote:

> Hello,
> Please review the fix for jdk10.
>
> The EventListenerList class was implemented to be thread safe. To achieve
> the correct state of the object:
>  - two mutators(add/remove) were marked as synchronized.
>  - the internal array is updated via copy-on-write, the internal array is
> never modified after it was set.
>
> It was assumed that the getXXX methods will get the value which was already
> set by the mutator or the previous value if mutator is in-progress but was
> not update the array. The problem is that the getXXX is not necessary read
> the value which was set, because of lack of happens-before between
> add/remove and getXXX. In the fix the array was marked as volatile to solve
> the problem.
>
> The CSR will be created after the technical review (the field is protected
> in public class)
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-5031664
> Webrev can be found at: http://cr.openjdk.java.net/~serb/5031664/webrev.00
>
>
> --
> Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10] Review Request: 5031664 Increase thread safety of EventListenerList

Sergey Bylokhov
Thanks Alex and Andrej for a feedback.
I have created an updated CSR for this.
https://bugs.openjdk.java.net/browse/JDK-8191167

On 09/11/2017 23:04, Andrej Golovnin wrote:

> Hi Sergey,
>
> the order of modifiers is wrong. It should be "protected transient
> volatile". See http://cr.openjdk.java.net/~alundblad/styleguide/index-v6.html#toc-modifiers
> for details.
>
> Best regards,
> Andrej Golovnin
>
> On Fri, Oct 27, 2017 at 1:18 AM, Sergey Bylokhov
> <[hidden email]> wrote:
>> Hello,
>> Please review the fix for jdk10.
>>
>> The EventListenerList class was implemented to be thread safe. To achieve
>> the correct state of the object:
>>   - two mutators(add/remove) were marked as synchronized.
>>   - the internal array is updated via copy-on-write, the internal array is
>> never modified after it was set.
>>
>> It was assumed that the getXXX methods will get the value which was already
>> set by the mutator or the previous value if mutator is in-progress but was
>> not update the array. The problem is that the getXXX is not necessary read
>> the value which was set, because of lack of happens-before between
>> add/remove and getXXX. In the fix the array was marked as volatile to solve
>> the problem.
>>
>> The CSR will be created after the technical review (the field is protected
>> in public class)
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-5031664
>> Webrev can be found at: http://cr.openjdk.java.net/~serb/5031664/webrev.00
>>
>>
>> --
>> Best regards, Sergey.


--
Best regards, Sergey.