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

[John Hendrikx]

On Mon, 3 Mar 2025 20:44:08 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> I haven't been tracking these fixes for allowing initialization in background threads, but it seems to me that basically anything should be allowed as long as you're not part of a visible scene graph -- and I think there's also no expectation that all functionality of a control "works" as long as it is not yet part of such a graph (ie. the listeners are only needed once it is part of a scene graph).
>> 
>> If you make listeners conditional on being part of a scene graph, then I think you can handle these with a single code path.  Such a condition would be:
>> 
>>     ObservableValue<Boolean> partOfSceneGraph = node.sceneProperty()
>>       .flatMap(Scene::windowProperty)
>>       .flatMap(Window::showingProperty)
>>       .orElse(false);
>>       
>>       // The node here is "this", the chart, because as soon as it is part of the scene
>>       // graph, you want the listeners to function.
>>       
>> You can then safely install any listeners directly, regardless if they're on a background thread or not:
>> 
>>     someProperty().when(partOfSceneGraph).subscribe( ... );
>> 
>> Or:
>> 
>>     someProperty().when(partOfSceneGraph).addListener( ... );
>> 
>> Or with any mappings added, you can even pick where you put the `when`:
>> 
>>     someProperty().flatMap(xyz).when(partOfSceneGraph).addListener( ... );
>>     someProperty().when(partOfSceneGraph).flatMap(xyz).addListener( ... );
>> 
>> 
>> On a background thread, `partOfSceneGraph` will be `false`, and no listener gets installed (yet).  As soon as it becomes `true` all listeners with that same condition become active.  It becomes `true` automatically when the node has a scene belonging to a window that is visible.  Vice versa, if it ever becomes `false` again, (which it may when the Node is removed or replaced) all listeners using this condition are removed again.
>
> Thank you!
> 
> This is not what I asked though: my question was about using `Subscription` in one-off case, and more specifically, about whether it is even possible to unsubscribe from within the lambda.
> 
> What you are proposing is a slightly more involved change: for example, registering and unregistering listeners each time might introduce other issues (especially since we have no way to prioritize listeners/handlers).  It also complicates the management of skin (as skins can be added/removed), something I would rather avoid.

As for this current code:

                ObservableValue<Window> winProp = sceneProperty().flatMap(Scene::windowProperty);
                accessibilityActive = winProp; // keep the reference so it won't get gc
                Subscription sub = winProp.subscribe((win) -> {
                    if (win != null) {
                        if (accessibilityActive == winProp) {
                            accessibilityActive = null;
                        }
                        if (isAccessibilityActive()) {
                            handleAccessibilityActive(true);
                        }
                        //winProp.removeListener(this);
                        sub.unsubscribe(); <-- COMPILE ERROR
                    }
                });
                
What you want there is not possible in this way as you can't use `this` in a lambda, nor refer to it via a local.  You can achieve it only if you store the subscription in a field, not in a local.  Assigning the Lambda to a ChangeListener local, and using a ChangeListener may be simpler (unless you go for the `when` solution that may eliminate the need for 2 different code paths).

Note that there is no need to store `winProp`; `flatMap` does not use weak listeners (when in active use), and so as long as `sceneProperty()` exists, the derived property via `flatMap` will also exist, and also its listener.  The way the fluent mapping system works is that listeners are only present when something is actually listening:

    ObservableValue<Window> winProp = sceneProperty().flatMap(Scene::windowProperty);
    // ^^ nobody is listening, so no listeners installed and no hard references; it could GC but
    // it is currently in a local, so it won't
    
    ObservableValue<Window> winProp = sceneProperty().flatMap(Scene::windowProperty);
    winProp.subscribe(v -> { .. });
    // ^^ somebody is listening; all listeners get installed automatically, and now there are
    // hard references.  No risk of GC.
    
You can then write it as a single statement which won't be GC'd as long as the subscription is active:
    
    subscriptionFIeld = sceneProperty()
        .flatMap(Scene::windowProperty);
        .subscribe(v -> { 
            // use subscription field here if you want to unregister the lambda
            // Note: when you do, there will again be no listeners and thus no hard references and
            // thus the intermediate property created by flatMap can now be GC'd :)
        });

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

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