-
Notifications
You must be signed in to change notification settings - Fork 141
Add unsafe_close to ServiceClient #1210
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
base: main
Are you sure you want to change the base?
Conversation
| #[pymethods] | ||
| impl ClientRef { | ||
| fn drop_client(&mut self) { | ||
| self.retry_client = None |
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 don't think this is predictable enough. The goal is to guarantee that when unsafe_close returns, the socket is no longer connected. But in this case, if a call was being made or the client is held in some other way by some other Core aspect, the connection will remain.
I think we may have to provide an "unsafe close" on the Rust side that does a predictable channel drop and confirms it is the only one using the channel. Hopefully that's not too hard to do, I have not looked into it. This may require putting the channel in a non-cloneable wrapper and having an Arc of that. Or maybe you have to get access to the non-cloneable underneath part. Unsure.
| fn drop_client(&mut self) { | ||
| self.retry_client = None | ||
| } |
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 100% sure if this is sufficient. Clients are Clone themselves, and share the underlying connection. It's entirely possible that on the Core side we might clone the client somewhere and then dropping the client ref won't actually seal the deal.
That said, there's also no readily available way to actually close the underlying connections Core side either. So, we might have to just attach a small caveat here in the docstrings or something.
What was changed
ClientRef.retry_clientin an optionOption<Client>that returns aPyErrin the case it doesn't exist.retry_clientwith the new helper method.ClientRef.drop_client()method in Python viaServiceClient.unsafe_close()Why?
As described in #1202, some advanced users need to control the timing of closing TCP connections.
Checklist
Closes [Feature Request] Advanced, unsafe ability to close the client #1202
How was this tested:
Unit test was added to query connections owned by the process and verify changes in connection count.
Any docs updates needed?