Conversation
|
I reverted the first change (to call removeCloseListener from appender/tailer close methods) as a bunch of tests failed - will come back to. The second change gives us some test flakiness with tests occasionally failed with |
|
|
||
| @Override | ||
| protected void performClose() { | ||
| // queue.removeCloseListener(this); |
There was a problem hiding this comment.
see comments - will come back to
JerryShea
force-pushed
the
feature/closeables-weak-ref
branch
from
22eecac to
30eef4a
Compare
|
Kudos, SonarCloud Quality Gate passed! |
I vote definitely not b - unless it's done locally on the problematic tests, but if you're doing that you might as well fix them. "Creating an issue" no way guarantees it will ever get done, in the meantime, we are muting the thing we use to detect resource leaks so we will run the risk of introducing resource leaks. If it's not tested it's broken. |
| closers.add(key); | ||
| closers.removeIf(wrc -> { | ||
| final Closeable closeable = wrc.get(); | ||
| return (closeable != null) ? closeable.isClosed() : true; |
There was a problem hiding this comment.
We don't need to keep empty weak references around do we?
return closeable == null || closeable.isClosed()
There was a problem hiding this comment.
Sorry that's what it does, but perhaps refactor to less byzantine logic
| // See https://github.com/OpenHFT/Chronicle-Queue/issues/1455. As many unit tests do not use try/finally | ||
| // to manage tailer or appender scope properly. we are ignoring this exception for now. | ||
| // TODO: remove the below line and run the tests many times to flush out the problematic tests (or else inspect the codebase) | ||
| ignoreException("Discarded without closing"); |
nicktindall
left a comment
nicktindall
left a comment
There was a problem hiding this comment.
Don't silence leak detection
| @NotNull | ||
| private final BiFunction<RollingChronicleQueue, Wire, SingleChronicleQueueStore> storeFactory; | ||
| private final Set<Closeable> closers = Collections.newSetFromMap(new IdentityHashMap<>()); | ||
| private final Set<WeakReference<Closeable>> closers = Collections.newSetFromMap(new IdentityHashMap<>()); |
There was a problem hiding this comment.
Need to ensure these resources are closed if discarded, otherwise, the resources they reference won't be cleaned up except by further GCs, if they are closed on being discarded.
…ences not leaked after they would otherwise be eligible for GC. Closes #1455
JerryShea
force-pushed
the
feature/closeables-weak-ref
branch
from
30eef4a to
df20e3e
Compare
|
3 participants
WIP - do not merge
Fixes two problems:
Second point above we should definitely discuss.
If we can reach consensus on this then after this change has been merged, it will be possible to set
disable.discard.warning=falseto register a finalizer for both appenders and tailers and ensure that if the finalizer is called, that they will warn and close themselves, thus releasing any resources. Backporting this to 23 will not be trivial as discard warnings/finalizers were different back then.