VEuPathDB / VEuPathDB/FgpUtil

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

Open
#13 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
Dominant language
Java
Stars
0
Forks
2
PR merge metrics
No merged PRs in 30d

Description

@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.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by tracing client creation through the fgputil-client methods, especially makeAsyncRequest, JerseyClient, CloseableResponse, and ResponseFuture. Review how Jersey ClientBuilder clients are disposed and how asynchronous responses are represented. Done means every created Client has a clear close path without breaking the existing caller-facing API or async behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
api
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.