Skip to content
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

THRIFT-5283: add support for Unix Domain Sockets in lib/rs #2545

Merged
merged 1 commit into from Mar 30, 2022
Merged

THRIFT-5283: add support for Unix Domain Sockets in lib/rs #2545

merged 1 commit into from Mar 30, 2022

Conversation

tokcum
Copy link
Contributor

@tokcum tokcum commented Mar 11, 2022

Client: rs

The Thrift crate does not support Unix Domain Sockets (UDS). After reviewing and expressing my thoughts on THRIFT-5283, I added support for UDS.

This is not a breaking change. Inspired by how actix_web::HttpServer supports UDS, I added an additional fn to listen to UDS. I reorganized some other existing code to be able to reuse it for UDS. I also had to implement the trait TIoChannel for UnixStream. This went to transport::socket.

As UnixStream is only available on Unix, I tagged the code with #[cfg(unix)]. I also cross compiled lib/rs to Windows with the following cargo targets:

i686-pc-windows-gnu
i686-pc-windows-msvc
x86_64-pc-windows-gnu
x86_64-pc-windows-msvc

Last but not least I ran the rust <-> rust test harnesses as well as the cross test harnesses. This looks good. At least, my contribution didn't introduce new fails.

  • Did you create an Apache Jira ticket?
    -> already existed as THRIFT-5283
  • If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
  • Did you squash your changes to a single commit? (not required, but preferred)
  • Did you do your best to avoid breaking changes?
    -> not a breaking change.

@tokcum
Copy link
Contributor Author

tokcum commented Mar 15, 2022

I tried to figure out why one of the Travis CI jobs failed. To be honest, I've no clue. I do not see a connection to the PR.

@tokcum
Copy link
Contributor Author

tokcum commented Mar 17, 2022

Actually this PR is a blocker for my osquery-rust crate. I appreciate any feedback on how to bring this PR forward.

Copy link
Contributor

@allengeorge allengeorge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Suggest replacing use of comparison with "" with Option and if let for the UDS usages.

Thank you!

@tokcum
Copy link
Contributor Author

tokcum commented Mar 18, 2022

Looks good! Suggest replacing use of comparison with "" with Option and if let for the UDS usages.

Thank you!

Thanks Allen, will do and update PR.

@tokcum
Copy link
Contributor Author

tokcum commented Mar 18, 2022

Looks good! Suggest replacing use of comparison with "" with Option and if let for the UDS usages.

Thank you!

Surprisingly the linter discouraged me to use if let and pointed me to .is_none() just to complain again and proposing a match statement. This didn't happen when I compiled with cargo build but when I used make to run the tests. It looks like additional rules apply depending on which tools are used.

Just for reference, the two messages I received where:

error: redundant pattern matching, consider using `is_none()`

error: You checked before that `unwrap()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.

So I used the match statement.

@allengeorge allengeorge merged commit f033641 into apache:master Mar 30, 2022
@allengeorge
Copy link
Contributor

@tokcum Sorry for the delay, and thank you so much for your contribution!

@tokcum tokcum deleted the THRIFT-5283 branch April 13, 2022 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants