New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RESTEASY-3135] Initial change for removing finalizers. #3102
Conversation
c4f5121
to
0a74775
Compare
...main/java/org/jboss/resteasy/client/jaxrs/engines/ManualClosingApacheHttpClient43Engine.java
Show resolved
Hide resolved
6cc8431
to
0207710
Compare
https://issues.redhat.com/browse/RESTEASY-3135 Signed-off-by: James R. Perkins <jperkins@redhat.com>
@jamezp, can I just verify that I'm understanding "cleaning" correctly? It looks like
So, when MultipartInputImpl goes away, the Message becomes phantom reachable. |
@ronsigal That is how I'm hoping it works at least. The idea is that we don't keep |
@Override | ||
public void close() { | ||
cleanable.clean(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not clear about this. cleanable.clean() will set the AtomicBoolean closed to true. Then, if the CleanupAction gets called, closed.compareAndSet(false, true) returns false and client.close() doesn't happen. What am I missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When cleanable.clean()
is invoked it's unregistered from the cleaner so it won't be cleaned again. https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/ref/Cleaner.Cleanable.html#clean()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was stuck on
() -> closed.set(true)
instead of
ResourceCleaner.register(engine, new CleanupAction(closed, (CloseableHttpClient) client))
for some reason.
It makes sense now.
Looks good to me. What else needs to be done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you approved it, it just needs to be merged :)
https://issues.redhat.com/browse/RESTEASY-3135
This is not yet complete, but wanted to get some tests running.