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

[John Hendrikx]

On Tue, 4 Mar 2025 21:24:46 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

>> I think the problem is in the callbacks themselves, as they'd be on the wrong thread.  I vaguely remember experimenting with a system where you can provide an Executor for callbacks (like `Platform::runLater`) to mitigate this.
>
> 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());
    }
  }
}

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

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