On Sat, 11 Jul 2026 02:10:59 GMT, John Hendrikx <[email protected]> wrote:
>> 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 > everyth... ok, you convinced me. would it be possible to write a condensed version of this explanations in the test's comments? ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r3572155523
