On Thu, 7 Jul 2022 02:31:54 GMT, Nir Lisker <nlis...@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
