Ensure one-shot wrappers release their delegates #92928
Conversation
We recently adjusted `RunOnce`, `Releasables#releaseOnce` and `ActionListener#notifyOnce` so that once they have fired they drop the now-unnecessary reference to the delegate. This commit introduces tests to verify that this reference does genuinely become unreachable (i.e. available for garbage collection) as expected. It also fixes a bug in `ActionListener#notifyOnce` which caused us to unexpectedly retain a reference to the delegate 🤦 Relates elastic#92452 Relates elastic#92507 Relates elastic#92537
|
Hi @DaveCTurner, I've created a changelog YAML for you. |
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
This works reliably on my machine but my main question to reviewers is whether you think this will work everywhere our test suite expects to run. In particular, explicitly triggering the GC is not guaranteed to work in all cases, but it seems that it does in the test suite, right? |
|
Explicitly triggering the GC doesn't guarantee that the references will be marked as unreachable. I think it will work for us, but we need to be careful. MXBean.gc() is equivalent to System.gc() as far as I know. It will cause a GC on G1, but in most cases it will simply be a new space collection, which means it will only remove the references of the recently allocated objects. If there was high allocation pressure the collector may decide to tenure the object, e.g. the object survives at least one minor GC and it's tenured. In this scenario, the only way the reference will be cleaned up for certain is if we cause a full collect. The JDK internal tests have this If there's very little allocation between when we allocate the references and release them in our tests, I think this will work. |
| final ReachabilityChecker reachabilityChecker = new ReachabilityChecker(); | ||
|
|
||
| for (int i = between(1, 3); i > 0; i--) { | ||
| step.addListener(reachabilityChecker.register(ActionListener.wrap(() -> {}))); |
There was a problem hiding this comment.
Is there a possibility to extract the allocations of the ActionListeners into an array before we register them? That way we'll minimize the allocations between register calls and ensure that when we call GC we'll get our references cleaned up.
There was a problem hiding this comment.
If it needs that much ceremony to be stable then this is not going to work in general. I wonder, would it work to do the same thing that the G1OverLimitStrategy does?
There was a problem hiding this comment.
Yeah, I think if we can somehow reuse that, we are definitely more certain the references will be properly checked for liveness. I also think the 3 action listeners are not a lot of garbage, it won't be a problem if we called System.gc() on creation of the ReachabilityChecker. It's fine as-is with the GC-up-front change.
There was a problem hiding this comment.
Ok, I added a short comment to the class indicating that there is some possible flakiness here. We can revisit this if it turns out to be bad.
| final ReachabilityChecker reachabilityChecker = new ReachabilityChecker(); | ||
|
|
||
| for (int i = between(1, 3); i > 0; i--) { | ||
| future.addListener(reachabilityChecker.register(ActionListener.wrap(() -> {}))); |
There was a problem hiding this comment.
Same comment as above, moving as much allocation as we can ahead of time to make sure we don't hit a GC. Perhaps the most prudent thing to do in all these tests is call System.gc() before we go into any of the test code. With that we ensure that at least the new space in generational collectors (e.g. G1) will be collected.
|
|
||
| public void testReleaseOnceReleasesDelegate() { | ||
| final var reachabilityChecker = new ReachabilityChecker(); | ||
| final var releaseOnce = Releasables.releaseOnce(reachabilityChecker.register(() -> logger.info("test"))); |
There was a problem hiding this comment.
I think loggers tend to allocate a lot, if we could do something else with less allocation, it would be better.
There was a problem hiding this comment.
Fair point, a no-op method reference seems to work.
|
|
||
| public class ReachabilityCheckerTests extends ESTestCase { | ||
|
|
||
| public void testSuccess() { |
There was a problem hiding this comment.
I'd put System.gc() in a @before to make things more solid.
There was a problem hiding this comment.
I put a GC call in the ReachabilityChecker constructor; I'd like this to be more generally useful so I think requiring it in a @Before method is not going to work.
We recently adjusted
RunOnce,Releasables#releaseOnceandActionListener#notifyOnceso that once they have fired they drop the now-unnecessary reference to the delegate. This commit introduces tests to verify that this reference does genuinely become unreachable (i.e. available for garbage collection) as expected.It also fixes a bug in
ActionListener#notifyOncewhich caused us to unexpectedly retain a reference to the delegate 🤦Relates #92452
Relates #92507
Relates #92537