fix(gui-client): catch IPC connection dropouts as fatal errors #5795
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
test/multi-process-test
This unblocks some issues that need the IPC connection to be up
This reverts commit c2bdebd.
ReactorScram
commented
Jul 8, 2024
ReactorScram
changed the title
ReactorScram
changed the base branch from
main
to
refactor/gui-simplify-resource-updating
conectado
reviewed
Jul 10, 2024
Contributor
conectado
left a comment
conectado
left a comment
There was a problem hiding this comment.
Nice work fixing this, just left a few question/comments but nothing major
Comment on lines
527
to
535
| Req::Ipc(result) => match result { | ||
| Some(Ok(msg)) => if let Err(error) = self.handle_ipc(msg).await { | ||
| tracing::error!(?error, "`handle_ipc` failed"); | ||
| } | ||
| Some(Err(error)) => { | ||
| tracing::error!(?error, "IPC read failure"); | ||
| Err(Error::IpcRead)? | ||
| } | ||
| None => Err(Error::IpcClosed)?, |
Contributor
There was a problem hiding this comment.
I think it's clearer if you match directly over the result and reduce the indentation level, like:
Req::Ipc(Some(Ok(msg)) => {...}
Req::Ipc(Some(Err(Error)) => {...}
...| ctlr_tx.send(ControllerRequest::Ipc(msg)).await?; | ||
| loop { | ||
| let result = rx.next().await; | ||
| ctlr_tx.send(ControllerRequest::Ipc(result)).await?; |
Contributor
There was a problem hiding this comment.
I think we're depending on ctlr_tx being closed here some time after the channel in the line above is closed? But I would rather we use the while let(...) and explicitly inform ctlrx_tx about the channel closing.
Contributor
There was a problem hiding this comment.
and so we don't have to deal with deeply nested types like ControllerRequest<Result<Option<IpcServerMessage>>>
Signed-off-by: Reactor Scram <ReactorScram@users.noreply.github.com>
…pdating' into chore/catch-ipc-rx-error
Signed-off-by: Reactor Scram <ReactorScram@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #5760, refs #5790
Also removes some redundant IPC-related code that was nearby.
If you stop the IPC service, e.g. due to an update on Linux, it will say "IPC connection closed". This isn't ideal but at least the Client does catch it now, instead of failing on the next IPC send.