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

Improvements to ClientContext for ensureOpen #5258

Merged
merged 1 commit into from
Jan 22, 2025

Conversation

ctubbsii
Copy link
Member

These changes are small quality fixes to ensure that ClientContext.ensureOpen is used when it is needed, and not used when it isn't. This fixes an issue seen where the client RPC timeout value is being retrieved from a supplier in a thread pool when returning RPC transports after a client is closed. In these cases, ensureOpen does not need to be checked. However, there were a few context API methods where it was not checked but should have been.

Also, improved the close method to ensure close activities are only called at most once, and made private and renamed an internal method to get the client properties from the ClientInfo object, so it's more clear which properties the method is returning and isn't exposed for misuse.

These changes are small quality fixes to ensure that
ClientContext.ensureOpen is used when it is needed, and not used when it
isn't. This fixes an issue seen where the client RPC timeout value is
being retrieved from a supplier in a thread pool when returning RPC
transports after a client is closed. In these cases, ensureOpen does not
need to be checked. However, there were a few context API methods where
it was not checked but should have been.

Also, improved the close method to ensure close activities are only
called at most once, and made private and renamed an internal method to
get the client properties from the ClientInfo object, so it's more clear
which properties the method is returning and isn't exposed for misuse.
@ctubbsii ctubbsii added this to the 3.1.0 milestone Jan 14, 2025
@ctubbsii ctubbsii self-assigned this Jan 14, 2025
@ctubbsii
Copy link
Member Author

I'll merge this after #4894 is fully merged to main, so it doesn't introduce more conflicts than already exist from that PR.

@DomGarguilo
Copy link
Member

I'll merge this after #4894 is fully merged to main, so it doesn't introduce more conflicts than already exist from that PR.

#4894 should be fully merged up into main as of c0fa497

@ctubbsii ctubbsii merged commit e1038d4 into apache:3.1 Jan 22, 2025
8 checks passed
@ctubbsii ctubbsii deleted the review-clientcontext-ensureOpen branch January 22, 2025 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants