Fixes #222: A memory leak in DashboardData. #224
Conversation
| this.backends = stream(backendServicesRegistry.get().spliterator(), false) | ||
| private List<Backend> updateBackendsFromRegistry() { | ||
| if (this.backends != null) { | ||
| this.backends.forEach(Backend::unregister); |
There was a problem hiding this comment.
this could just be
if (this.backends != null) {
unregister();
}
| @@ -206,6 +225,13 @@ private Backend(BackendService application) { | |||
| this.responsesSupplier = new ResponseCodeSupplier(metrics, METER, prefix, true); | |||
| } | |||
|
|
|||
| void unregister() { | |||
| registeredOrigins.forEach(origin -> { | |||
There was a problem hiding this comment.
Why do we have a new field registeredOrigins that contains the same list as the existing field List<Origin> origin?
There was a problem hiding this comment.
This is so that we unregister only those origins that actually have been registered.
I noticed that in some test cases (DashboardDataSupplier) the origins list contained different origins to those from registeredOrigins. But this could have been just a unusual or unrealistic dependency injection in the test itself.
|
|
||
| dashbaord.unregister(); | ||
|
|
||
| verify(eventBus, times(4)).register(any(DashboardData.Origin.class)); |
There was a problem hiding this comment.
Shouldn't this be verifying that the origins were unregistered, not registered again?
|
Note, this is required as a bug fix for styx 0.7.n branches only. We will not cherry pick to styx 1.0 branch as it will have a brand new implementation for dashboard data supplier. |
|
Will cherry pick to 1.0 as the refactoring is likely to be delayed. |
Fixes #222, a memory leak in dashboard data supplier.
The leak was caused by
DashobardData.Originobjects being continuously registered to the message bus, but never unregistered.