RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v18]

[Nir Lisker]

On Thu, 7 Jul 2022 02:31:54 GMT, Nir Lisker <nlisker at openjdk.org> wrote:

>> John Hendrikx has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 27 additional commits since the last revision:
>> 
>>  - Merge branch 'openjdk:master' into feature/fluent-bindings
>>  - Add null checks in Subscription
>>  - Update copyrights
>>  - Move private binding classes to com.sun.javafx.binding package
>>  - Add note to Bindings#select to consider ObservableValue#flatMap
>>  - Fix bug invalidation bug in FlatMappedBinding
>>    
>>    Also fixed a secondary issue where the indirect source of the binding
>>    was unsubscribed and resubscribed each time its value was recomputed.
>>    
>>    Add additional comments to clarify how FlatMappedBinding works.
>>    
>>    Added test cases for these newly discovered issues.
>>  - Fix typos in LazyObjectBinding
>>  - Rename observeInputs to observeSources
>>  - Expand flatMap javadoc with additional wording from Optional#flatMap
>>  - Add since tags to all new API
>>  - ... and 17 more: https://git.openjdk.org/jfx/compare/62bcd33c...d66f2ba6
>
> I didn't get into the fine details of the GC discussion yet, but I have a few points I would like to share:
> 
> 1. @hjohn's example 4, which seems to be a point of some disagreement, is a _de facto_ issue. Even if the technicalities or semantics imply that this is correct behavior, StackOverflow is littered with "why did my listener suddenly stopped working?" questions that are caused by this, and I have fell into this pitfall more than once myself (by starting from something like example 2, then making a slight change that transforms the code into something like example 4).
>   I didn't look yet into the possible alternatives discussed here for dealing with this, but I do believe that going with "this behavior is technically correct" is not the best _practical_ decision, provided that changing it will be backwards compatible.
> 2. On the subject of `Disposer` and cleaning references in a background thread, the JDK has [Cleaner](https://docs.oracle.com/en/java/javase/18/docs/api/java.base/java/lang/ref/Cleaner.html) (there is a nice recent Inside Java [post](https://inside.java/2022/05/25/clean-cleaner/) about it), which seems to do what we need, at least abstractly. I didn't look at the code to see if it fits our needs exactly. It also uses a `ReferenceQueue` with `PhantomReference`s.
> 3. It is clear to me that whatever GC/referencing model we choose to go with eventually (with whatever combination of fluent vs. regular bindings, listeners vs. bindings, expressions vs. properties...), we will have to document what is expected of the user, and we will have to do it very well. Currently, there are some mentions in the docs of listeners holding strong references, and weak references being used in the implementation ("user beware"), but we will need to be a lot more explicit in my opinion. Something like @hjohn's set of examples or some snippets from this discussion would be a good starting point. If we don't want to commit to some detail, we should at least document the current behavior with a note that it might change. It's just too easy to mess this up (even before this PR was considered). I don't mind taking care of such a PR when an implementation is agreed upon.

> Additionally, can you file a docs bug to clarify the GC behavior of bindings and mappings as suggested by @nlisker in point 3 of [this comment](https://github.com/openjdk/jfx/pull/675#issuecomment-1176975103)?

@hjohn If the followup work on this issue is too much and you want to focus on the implementation, you can assign it to me.

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

PR: https://git.openjdk.org/jfx/pull/675