RUST-802 Support Unix Domain Sockets

[PureWhiteWu]

This pull request adds support for Unix Domain Sockets.

This pull request also removes some minor unnecessary Box::pin.

Implements #686

[PureWhiteWu]

I've tested this and this works fine now.

Could you please review & merge this and then release a new version?

This feature is very important for us!

Thanks!

[abr-egn]

Hi! Thanks for contributing!

The changes look good, I've authorized an evergreen run and assuming that passes (modulo known flakes) I'll merge this. I'll need to check in with the rest of the team about the release schedule and get back to you on that.

[abr-egn]

It looks like you missed some match arms:

[2023/07/05 15:06:05.949] error[E0004]: non-exhaustive patterns: `&client::options::ServerAddress::Unix { .. }` not covered
[2023/07/05 15:06:05.949]    --> src/trace.rs:36:15
[2023/07/05 15:06:05.949]     |
[2023/07/05 15:06:05.949] 36  |         match self {
[2023/07/05 15:06:05.949]     |               ^^^^ pattern `&client::options::ServerAddress::Unix { .. }` not covered
[2023/07/05 15:06:05.949]     |
[2023/07/05 15:06:05.949] note: `client::options::ServerAddress` defined here
[2023/07/05 15:06:05.949]    --> src/client/options/mod.rs:114:5
[2023/07/05 15:06:05.949]     |
[2023/07/05 15:06:05.949] 101 | pub enum ServerAddress {
[2023/07/05 15:06:05.949]     |          -------------
[2023/07/05 15:06:05.949] ...
[2023/07/05 15:06:05.949] 114 |     Unix {
[2023/07/05 15:06:05.949]     |     ^^^^ not covered
[2023/07/05 15:06:05.949]     = note: the matched value is of type `&client::options::ServerAddress`
[2023/07/05 15:06:05.949] help: ensure that all possible cases are being handled by adding a match arm with a wildcard pattern or an explicit pattern as shown
[2023/07/05 15:06:05.949]     |
[2023/07/05 15:06:05.949] 37  ~             Self::Tcp { port, .. } => Some(port.unwrap_or(DEFAULT_PORT)),
[2023/07/05 15:06:05.949] 38  ~             &client::options::ServerAddress::Unix { .. } => todo!(),
[2023/07/05 15:06:05.949]     |
[2023/07/05 15:06:07.540] For more information about this error, try `rustc --explain E0004`.

[PureWhiteWu]

Sorry, seems that I've missed some of the features.
I will fix them now.

[PureWhiteWu]

Fixed now.

[abr-egn]

Looks good! Tagging in Isabel while evergreen finishes its run.

[PureWhiteWu]

Seems that there's still some error with the ci, but I'm not able to see the log.
Would you please tell me what's wrong so I can fix it?

[PureWhiteWu]

Seems that those tests had been failed for a long time?

[abr-egn]

Seems that those tests had been failed for a long time?

Yeah, the failures it's showing are known flakes or other issues and nothing to do with your PR :)

[isabelatkinson]

Thanks for the contribution! I have a few requests/suggestions. We should also unskip the spec tests for this (src/client/options/test.rs:46).

[isabelatkinson]

Can you please revert this change? We did this intentionally to reduce the size of the futures returned by these methods (RUST-793, #417).

[PureWhiteWu]

Thanks for this explanation!
I wonder if this can bring performance gain if we reduce the size of futures?
AFAIK, using Box here will bring runtime cost and make the compiler unable to inline this future.

[isabelatkinson]

Thanks for addressing my comments! It looks like the tests still need to be unskipped in the file I mentioned in my above comment. You can grep src/client/options/test.rs for "unix" and remove the skip logic for Unix platforms. The following compilation failures are also showing up in our Windows CI tasks:

 [2023/07/06 16:01:14.849]     --> src\client\options\mod.rs:1624:13

 [2023/07/06 16:01:14.849]      |

 [2023/07/06 16:01:14.849] 1624 |             Ok::<(), crate::error::Error>(())

 [2023/07/06 16:01:14.849]      |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `Result<(), ErrorKind>`, found `Result<(), Error>`

 [2023/07/06 16:01:14.849]      |

 [2023/07/06 16:01:14.849]      = note: expected enum `std::result::Result<_, error::ErrorKind>`

 [2023/07/06 16:01:14.849]                 found enum `std::result::Result<_, error::Error>`

 [2023/07/06 16:01:14.849] note: return type inferred to be `std::result::Result<(), error::ErrorKind>` here

 [2023/07/06 16:01:14.849]     --> src\client\options\mod.rs:1620:24

 [2023/07/06 16:01:14.849]      |

 [2023/07/06 16:01:14.849] 1620 |                   return Err(ErrorKind::InvalidArgument {

 [2023/07/06 16:01:14.849]      |  ________________________^

 [2023/07/06 16:01:14.849] 1621 | |                     message: "Unix domain sockets are not supported on this platform".into(),

 [2023/07/06 16:01:14.849] 1622 | |                 });

 [2023/07/06 16:01:14.849]      | |__________________^

 [2023/07/06 16:01:17.337] For more information about this error, try `rustc --explain E0308`.

 [2023/07/06 16:01:17.337] error: could not compile `mongodb` due to previous error

 [2023/07/06 16:01:17.337] warning: build failed, waiting for other jobs to finish...

 [2023/07/06 16:01:32.564] error: could not compile `mongodb` due to previous error

[PureWhiteWu]

@isabelatkinson Sorry, I missed that part about tests in the previous comment, I've added them on unix platform.
There's still something wrong with the spec tests about unix domain sockets, but I've checked and found that it's nothing to do with the unix domain socket part itself.
The problems are that:

1. when parsing connection string such as mongodb://host/admin, should the auth be set to the following?

auth: {
    "username": null,
    "password": null,
    "db": "admin"
}

2. when parsing connection string such as mongodb://host/admin?w=1 which "w" is unknown, should the parser return error?

I didn't find similar tests for ipv4, and I think those tests may also fail on ipv4.

I'm not quite familiar with the mongodb specs, could you please help me to fix those tests? Thanks!

[isabelatkinson]

1. Yes, the string that comes after / (and before ? if options are present) should be considered the auth database. It is an error for the connection string to contain more than one / (excluding the :// at the beginning) to avoid any ambiguity around which part of the string is the auth database, which is why unix domain sockets must be percent-encoded in a connection string. This diagram showing the structure of a connection string (from this spec) should give some more context:

2. Yes, the parser should return an error if an unknown option is specified. w, however, is a known option. The full list of supported URI options is defined here.

Let me know if you have more specific details about the failures you're encountering and I'd be happy to help you look into them further!

[PureWhiteWu]

Hi, thank you very much!
I've checked those tests and found that they also fails under ipv4 situation(I've changed the test to ipv4 locally and it also fails), so I think it's not related to the unix domain socket support.
Could you please help me to check this conclusion, if it's true I think we can skip those tests for now, and we may need to fix them later maybe in another pr?

[PureWhiteWu]

Branch rebased to latest main.

[isabelatkinson]

Hi @PureWhiteWu, I will be OOO for the rest of the week but I will get back to you next week!

[isabelatkinson]

Hi @PureWhiteWu, I spent some time running these tests locally and fixed a couple of unrelated bugs that were causing the errors you're seeing. Would you mind if I pushed the changes up to your branch? (You'll of course still get credit for this PR in our git history).

[PureWhiteWu]

@isabelatkinson Certainly not! Feel free to change anything on this branch~

[isabelatkinson]

Hi @PureWhiteWu, I've pushed a few changes that have fixed up the errors you were seeing. Thank you again for your contribution! I'm going to tag in @abr-egn for one final review and then this should be good to merge 🙂

[PureWhiteWu]

@isabelatkinson Thank you very much for helping me to fix these errors!

[abr-egn]

LGTM!

[abr-egn]

Thank you again! This will go out with our next release.