RFR: 8349091: Charts: exception initializing in a background thread [v6]

[John Hendrikx]

On Tue, 4 Mar 2025 22:59:15 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> The listener callbacks will always be on the thread that mutates the property being listened to, so that isn't the problem. The problem in this case is that adding or removing a listener while another thread is modifying the property, which will iterate the list of listeners, is not thread-safe. Consider the following:
>> 
>> 
>>         final var prop = new SimpleBooleanProperty(false);
>> 
>>         // add listeners in a background thread
>>         var thr = new Thread(() -> {
>>             for (int i = 0; i < 10000; i++) {
>>                 ChangeListener<Boolean> listener = (obs, o, n) -> {
>>                     if (!mainThread.equals(Thread.currentThread())) {
>>                         System.err.println("***** ERROR: listener called on wrong thread: " + Thread.currentThread());
>>                     }
>>                 };
>>                 prop.addListener(listener);
>>             }
>>         });
>>         thr.start();
>> 
>>         // Fire events in main thread
>>         for (int jj = 0; jj < 10000; jj++) {
>>             prop.set(!prop.get());
>>         }
>> 
>> 
>> The listeners all get called on the (correct) main thread, but continually adding listeners while the property is modified will lead to AIOOBE or NPE (the above example gets random NPEs when I run it).
>
> Yes, but that's because there is no synchronization.  Here is a version that does do synchronization; I see no more exceptions (and it runs just as fast really):
> 
> 
> public class PropTest {
>   public static void main(String[] args) {
>     Thread mainThread = Thread.currentThread();
>     final var prop = new SimpleBooleanProperty(false) {
>       @Override
>       public synchronized void addListener(ChangeListener<? super Boolean> listener) {
>         super.addListener(listener);
>       }
> 
>       @Override
>       public synchronized void set(boolean newValue) {
>         super.set(newValue);
>       }
>     };
> 
>     // add listeners in a background thread
>     var thr = new Thread(() -> {
>         for (int i = 0; i < 1000000; i++) {
>             ChangeListener<Boolean> listener = (obs, o, n) -> {
>                 if (!mainThread.equals(Thread.currentThread())) {
>                     System.err.println("***** ERROR: listener called on wrong thread: " + Thread.currentThread());
>                 }
>             };
>             prop.addListener(listener);
>         }
>     });
>     thr.start();
> 
>     // Fire events in main thread
>     for (int jj = 0; jj < 1000000; jj++) {
>         prop.set(!prop.get());
>     }
>   }
> }

Of course for a generic solution, we could provide a wrapper for properties, or custom synchronized properties.

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1697#discussion_r1980382245