-
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,11 +17,12 @@ use crate::runtime; | |
|
|
||
| pyo3::create_exception!(temporal_sdk_bridge, RPCError, PyException); | ||
|
|
||
| type Client = RetryClient<ConfiguredClient<TemporalServiceClientWithMetrics>>; | ||
| type TemporalClient = ConfiguredClient<TemporalServiceClientWithMetrics>; | ||
| type Client = RetryClient<TemporalClient>; | ||
|
|
||
| #[pyclass] | ||
| pub struct ClientRef { | ||
| pub(crate) retry_client: Client, | ||
| retry_client: Option<Client>, | ||
| pub(crate) runtime: runtime::Runtime, | ||
| } | ||
|
|
||
|
|
@@ -95,10 +96,13 @@ pub fn connect_client<'a>( | |
| let runtime = runtime_ref.runtime.clone(); | ||
| runtime_ref.runtime.future_into_py(py, async move { | ||
| Ok(ClientRef { | ||
| retry_client: opts | ||
| .connect_no_namespace(runtime.core.telemetry().get_temporal_metric_meter()) | ||
| .await | ||
| .map_err(|err| PyRuntimeError::new_err(format!("Failed client connect: {err}")))?, | ||
| retry_client: Some( | ||
| opts.connect_no_namespace(runtime.core.telemetry().get_temporal_metric_meter()) | ||
| .await | ||
| .map_err(|err| { | ||
| PyRuntimeError::new_err(format!("Failed client connect: {err}")) | ||
| })?, | ||
| ), | ||
| runtime, | ||
| }) | ||
| }) | ||
|
|
@@ -117,23 +121,43 @@ macro_rules! rpc_call { | |
|
|
||
| #[pymethods] | ||
| impl ClientRef { | ||
| fn drop_client(&mut self) { | ||
| self.retry_client = None | ||
| } | ||
|
Comment on lines
+124
to
+126
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not 100% sure if this is sufficient. Clients are 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. |
||
|
|
||
| fn update_metadata(&self, headers: HashMap<String, RpcMetadataValue>) -> PyResult<()> { | ||
| let (ascii_headers, binary_headers) = partition_headers(headers); | ||
| let client = self.configured_client()?; | ||
|
|
||
| self.retry_client | ||
| .get_client() | ||
| client | ||
| .set_headers(ascii_headers) | ||
| .map_err(|err| PyValueError::new_err(err.to_string()))?; | ||
| self.retry_client | ||
| .get_client() | ||
| client | ||
| .set_binary_headers(binary_headers) | ||
| .map_err(|err| PyValueError::new_err(err.to_string()))?; | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| fn update_api_key(&self, api_key: Option<String>) { | ||
| self.retry_client.get_client().set_api_key(api_key); | ||
| if let Ok(client) = self.configured_client() { | ||
| client.set_api_key(api_key); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl ClientRef { | ||
| pub(crate) fn retry_client(&self) -> Result<&Client, PyErr> { | ||
| self.retry_client | ||
| .as_ref() | ||
| .ok_or_else(|| PyRuntimeError::new_err("client has been dropped")) | ||
| } | ||
|
|
||
| fn configured_client(&self) -> Result<&TemporalClient, PyErr> { | ||
| self.retry_client | ||
| .as_ref() | ||
| .map(RetryClient::get_client) | ||
| .ok_or_else(|| PyRuntimeError::new_err("client has been dropped")) | ||
| } | ||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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_closereturns, 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
Arcof that. Or maybe you have to get access to the non-cloneable underneath part. Unsure.