On Tue, 4 Mar 2025 22:59:15 GMT, John Hendrikx <jhendr...@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
