-
Notifications
You must be signed in to change notification settings - Fork 85
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
feat(daemon): Remove named_pipe
in favor of tokio
#1710
Conversation
Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Trae Yelovich <[email protected]>
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## master #1710 +/- ##
=======================================
Coverage 92.74% 92.74%
=======================================
Files 416 416
Lines 6916 6916
Branches 1221 1221
=======================================
Hits 6414 6414
Misses 501 501
Partials 1 1 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Trae Yelovich <[email protected]>
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.
Just one question
Signed-off-by: Trae Yelovich <[email protected]>
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.
Thanks for migrating to tokio
for technical currency 👍 Left a few questions 🙂
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 agree with Timothy's comment that it will be useful to confirm whether there is any performance impact related to connecting first or checking for daemon first.
I also had one question about confirming the behavior during shutdown of the communication stream.
Perhaps neither question will require any change. Everything else looks good to me.
Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Trae Yelovich <[email protected]>
Kudos, SonarCloud Quality Gate passed! |
Release succeeded for the The following packages have been published:
Powered by Octorelease 🚀 |
The
named_pipe
library is no longer maintained, and the recommendation from the library's author is to usetokio
's implementation of named pipes instead.This PR replaces the
named_pipe
library withtokio
, and also usestokio::net::UnixStream
instead of the standard library'sUnixStream
so that logic is consistent across all platforms for the communication functions.It also updates the daemon logic so that the Rust client looks for the Node.js server process before trying to connect to the socket/pipe. This prevents race conditions where the server process was started by the daemon, but the Rust client tried to attach to the pipe before it was initialized by the server.