🧵 zb: Use async versions of UnixStream when possible - #1798
Conversation
9ef832c to
318bda4
Compare
|
@zeenix Can't check if windows functions correctly since I don't have it installed atm but I think everything should work fine. If you can give it a look and tell me if anything is not right. I've also moved the connect code to unix.rs since that is what is being done with Tcp and Unixexec and seemed easier to maintain than a lot of code in mod.rs. |
2e24e0e to
4b8e92e
Compare
|
@zeenix So, can this be merged? I've been using the fork on my PC for a personal project and all seems to work fine. There was an issue with windows but it should be solved now. |
I was on a work trip this week so didn't get to review but in any case, please be patient.
Great to hear.
Cool. I'll check. @elmarco could you please look at this from Windows' perspective? |
Multiple CI failures here, so this will need more work for sure. :) |
Sure thing!
Yeah, I'll check again to see what's wrong here. |
|
@zeenix The CI issues should be fixed now. |
zeenix
left a comment
There was a problem hiding this comment.
Apart from these, please also choose a more appropriate emoji. ♻️ is for refactoring and that's not a good catagorization of the change.
|
Addressed all of the comments. Not really sure what emoji I should use for the main commit though 🤔. Maybe ⚡️? Though I'm not sure since there is not a clear performance improvement. |
Thanks. I unresolved one though. I'm sorry to ask you to do more but I did describe the reason for the changes I'm asking for.
I think it's more appropriate than ♻️ still and I think there is a tiny performance improvement from not launching threads. Having said that 🧵is probably the best here. |
Oh and the windows CI continues to be failing. |
|
Switched out the order of the commits. Going to find a windows PC to test out why its failing. |
|
@zeenix Not really sure if I like how that second commit turned out, but everything should be done now. Windows turned out to be more of a pain than I imagined. |
7b21f3a to
dd22bde
Compare
Ouch, sorry! I actually was considering to drop the Windows support as I also don't have a machine to test it on but some people do use zbus on Windows. 🤷 |
Move UnixSocket connection code to unix.rs from mod.rs for better readability and maintainability.
zeenix
left a comment
There was a problem hiding this comment.
Great work! I appreciate your patience and perseverance. 👍 I'm pretty sure the next iteration can be merged.
| } | ||
|
|
||
| #[cfg(any(unix, not(feature = "tokio")))] | ||
| async fn get_stream(self) -> Result<UnixStream> { |
There was a problem hiding this comment.
I don't think we need this change to be so intrusive and remove functions just added in the previous commit. Why not just simplify this method instead?
There was a problem hiding this comment.
I've readded the function and made it windows specific since from a maintainability point of view it seems the best for the eventual windows specific code removal.
There was a problem hiding this comment.
seems the best for the eventual windows specific code removal.
And to be fair, the UDS on windows support for Rust does seem to be going along quite well so maybe it's not so far off after all :D
When connecting to a UnixSocket a blocking/threaded API is being used. This commit changes this to use `tokio::net::UnixStream` and `Async<std::os::unix::net::UnixStream>` when possible preventing the use of extra threads. The windows version continues using the blocking/threaded API. The `UnixStream::connect_addr` function from `SocketAddrExt` is no longer used since neither tokio nor async-io supports it. Instead, we use the plain `connect` functions in both environments, since they allow for abstract names when the path is prepended with '\0' (a null byte).
Great to hear! I rather appreciate the strict guidelines and expectation of code quality. That what makes open source have some of the best software out there! |
| // This is a `path` in case of Windows until uds_windows provides the needed API: | ||
| // https://github.com/haraldh/rust_uds_windows/issues/14 | ||
| #[cfg(any(unix, not(feature = "tokio")))] | ||
| fn take_addr(self) -> Result<PathBuf> { |
There was a problem hiding this comment.
you're renaming a method in the last commit that you created in the previous one and the commit still seems more intrusive than it should be IMO. This commit should mostly just be replacing spawn_blocking use for unix in get_stream (which should exist for both configs) so the caller is simply abstracted from creation of stream and this commit becomes simpler and very much to the point.
Well? |
|
Ping? You were so close. |
When connecting to a
UnixSocketa blocking/threaded API is being used. This commit changes this to usetokio::net::UnixStreamandAsync<std::os::unix::net::UnixStream>when possible, preventing the use of extra threads. The windows version continues using the blocking/threaded API.The connect_addr API is dropped since neither tokio nor async-io supports it. Instead, we use the connect functions in both environments, since they allow for abstract names when the path is prepended with '\0'.
Move the connection code to unix.rs for better future maintainability.
Closes #1796