On Fri, 10 Jul 2026 22:15:29 GMT, Andy Goryachev <[email protected]> wrote:
>> I realize I didn't truly answer your question. This is not specified >> anywhere (the whole weak listener removal mechanism is basically internal >> stuff); there is only an indirect expectation that if you override >> add/remove listeners in order to track listeners (and by that I only mean >> empty or not empty, not a counter) that you would get the correct result. >> >> So this tests the implementation I did, to ensure it is compatible with >> existing JavaFX code that tracks an `observed` flag. >> >> I think improving this system somehow (more eagerly removing dead listeners) >> would require also a way to notify interested parties whether the list has >> become empty. >> >> I believe an earlier iteration tried to do this by also returning a >> `boolean` from the notify call (ie. it would return true if all listeners >> were removed during notification), but I think that ran into a wall. I think >> the problem with that approach was that JavaFX code really doesn't like it >> when a simple notify (normally harmless) has a lot of consequences (like all >> kinds of listeners getting removed in a whole chain because something went >> from observed to unobserved state). > > the reason I asked this question is that we probably should test the public > APIs, not the implementation specifics. if, for example, one day there is > some change in the implementation that still conforms to the spec, the test > might/will fail. > > in this case, the implementation might be changed to eagerly clear empty weak > references, then what? there might still be some value in having this test, > perhaps a comment should be added for anyone who comes after us to know that > this test assumes a particular internal implementation. There is no public specification for this, and there can't be one as we don't want users to rely on any kind of behavior surrounding this so we can keep our options open (never mind that weak references are notoriously unpredictable, and vary by JVM implementation). At most there is an internal contract here (copied from `ExpressionHelper`), and that contract says: don't remove listeners when it is not expected (ie. during notify, or "randomly" with a background thread or something). Removals should ideally always go through `addListener` and `removeListener`, but we've compromised there (for now) that removals are also allowed if the listener list is being modified anyway (again this is from `ExpressionHelper`). So what would break if you did change it? You'd break the code I showed you, lazy bindings, and probably a few other things. Any high level test against the API will have a tough time catching this, as unpredictable weak references are involved. Also such tests would make no sense either then, as you'd be fiddling in the test to make this "break" (ie. calling `System.gc()`) but nothing in the API would hint that you'd need to do this (it is unspecified after all), so the test would again be implementation specific (which BTW many tests are, or at least they're testing against internal API, internal expectations, of which FX has many; not having a public spec doesn't mean you can't test a class on an internal expectation). So to ensure this doesn't break by accident, and to be sure at least ONE test fails (because there is a good chance everything else will pass), this specific test is there to verify the unwritten contract we have with the our own inner workings of properties (not something the user needs to know). Also note, without weak listeners, the only possible way to remove a listener is via `removeListener`. It is quite reasonable to expect property implementations that they could "track" their listener count by overriding `addListener` and `removeListener`. Nothing on `removeListener` says "listeners may disappear at any time if they were wrapped in a weak listener and we'll not even call `removeListener`, hah!" -- so much for perhaps tracking other stuff surrounding listeners (resources, debugging, etc). One could say that the ball was dropped here when weak listeners were added; removal should never have occurred without going through the proper API's; these API's are after all public. It only works for us currently because everything uses `ExpressionHelper` and almost nothing implements their own listener tracking (even though you can easily do this if you implement say `ObservableValue` yourself, as users would be forced to do as they don't have access to `ExpressionHelper`). So, the solution, out of scope for this, would be to not remove listeners silently, but call `removeListener`. It still remains to be seen if it is a good idea to do this **during** notification (as that's the only place they're currently not automatically removed) as that is sort of goes against the recommendation to not remove or add listeners during listener callbacks... ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r3562904124
