Skip to content
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

ClientUtil does not close Jersey Client objects, possibly resulting in resource leak #13

Open
ryanrdoherty opened this issue Oct 7, 2022 · 2 comments
Labels
bug Something isn't working

Comments

@ryanrdoherty
Copy link
Member

@dmgaldi discovered that we are creating new Client objects with every request to a number of methods in the fgputil-client package. Relevant Slack discussion below:

Daniel Galdi
I was looking at the makeAsyncRequest API in FGPUtil trying to learn a bit about jersey. I noticed that we create a jakarta.ws.rs.client.Client each time it's called. We might want to share one or a few clients instead of constructing so many:
Clients are heavy-weight objects that manage the client-side communication infrastructure. Initialization as well as disposal of a Client instance may be a rather expensive operation. It is therefore advised to construct only a small number of Client instances in the application. Client instances must be properly closed before being disposed to avoid leaking resources.

Ryan Doherty
Good point Dan, I didn't notice the expense there, though maybe that's a reason not to worry about it? What concerns me more is that we aren't closing these (ever, best I can tell). If you look at the code, there is a bunch of shutdownHook logic in the JerseyClient class, but it's not obvious that it helps us.
I looked around the fgputil client API and I can't see a good way to be backward compatible and also close this up. I was hoping we could send the Client along into a CloseableResponse (further possibly nested inside a ResponseFuture for async requests), but we currently deal in the opaque Future object returned by Jersey's native async method.
So, we should probably pass in the Client, and let the caller figure out how to close it. Starting in Jersey 2.1, it became AutoCloseable, so as a one-off callers can wrap ClientBuilder.newClient() in a try-with-resources. Our EDA services can maybe create one per incoming request that is shared for all its outgoing calls during that request; then we don't have to manage them at any higher level.

@ryanrdoherty ryanrdoherty added the bug Something isn't working label Oct 7, 2022
@dmgaldi
Copy link
Contributor

dmgaldi commented Oct 11, 2022

We discussed this in a fut on 10/11. One possible short-term mitigation is to make the client static or static/thread-local in FgpUtil but there were concerns about lifecycle management of the client. We came to the conclusion that we want to look into using Java 11's native HttpClient in place of the Jersey client in order to have a more fluid API. If the Java 11 HttpClient is more satisfactory, we may not need the FgpUtil ClientUtil at all for generating http requests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants