Add joining to increase reliability when closing[CPP-237]. #91
Conversation
john-michaelburke
force-pushed
the
john-michaelburke/wait_for_backend
branch
8 times, most recently
from
62a017f to
0c3914e
Compare
john-michaelburke
force-pushed
the
john-michaelburke/wait_for_backend
branch
from
0c3914e to
cf1c7b1
Compare
|
|
||
| if let Some(recv) = self.client_recv.take() { | ||
| drop(recv); | ||
| } |
There was a problem hiding this comment.
I also had a join here for the thread spawned in "fetch_message" but it causes an infinite hang on the Macos frontend benchmark whenever we try to kill the app although it works fine with linux/windows 😢 . Nonetheless I think I am seeing less crashes without the join. I am setting things such that the thread should end but definitely not ideal without the join.
| struct ServerEndpoint { | ||
| server_send: Option<mpsc::Sender<Vec<u8>>>, | ||
| } | ||
| impl Drop for ServerEndpoint { |
There was a problem hiding this comment.
I don't think we need any explicit drop implementations on the Server/SeverEndpoint type because fundamentally as long as the Server object is held by the Python code, this drop implementation isn't going to run. It order for drop to run, the Python code would need to explicitly detect the object, a better option is to just request termination, and then to do a blocking wait for termination to occur.
| impl Drop for ServerEndpoint { | ||
| fn drop(&mut self) { | ||
| if let Some(sender) = self.server_send.take() { | ||
| drop(sender); |
There was a problem hiding this comment.
This should be in a helper function exposed to Python, so that Python can indicate that it's done with the server
| fn drop(&mut self) { | ||
| self.is_running.set(false); | ||
|
|
||
| if let Some(recv) = self.client_recv.take() { |
There was a problem hiding this comment.
I don't think we need to explicitly drop the receiver, the close of the sender should be reflected as an error when we attempt to receive, then we can break of the receive loop on that error condition
| if !is_running.get() { | ||
| break; | ||
| } | ||
| let buf = server_recv.recv(); |
There was a problem hiding this comment.
I think all you need to do is add a timeout here, and then move the drop for ServerEndpoint into a method exposed to Python, no need for the is_running variable, just use the closure of the recv endpoint to signal that the thread should exit
| } | ||
| }; | ||
| match message { | ||
| m::message::Status(Ok(cv_in)) => { |
There was a problem hiding this comment.
This shouldn't be necessary if we just close the sender as mentioned above
| recv_thread.start() | ||
| app.exec_() | ||
| data_model.status(ApplicationStates.CLOSE) | ||
| with running_lock: |
There was a problem hiding this comment.
Should be able to call something like messages_main.stop_server() then just call the recv_thread.join(), closing the sender end of the mpsc should cause the loop in fetch_messages to terminate
|
This work was moved to #93. Closing this PR. |
2 participants
Implements
Drop for Server attempts to join the main backend threadConcerns