Fix an inconsistency in Linux version of TcpListener::accept

[hashmap]

If TcpListener is in non-blocking mode then accepted streams inherit
this property on all platforms except Linux, where accept4 is used
instead of accept.

This fix checks if a listener is in non-blocking mode and pass the
corresponding flag to accept4.

Fixes #67027

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Kimundi (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[sfackler]

That implementation is racy with concurrent changes to the blocking-ness of the socket. It also doubles the number of system calls per accept call.

We generally try to expose the system primitive without a lot of extra stuff on top. "TcpListener::accept calls accept4(2)" is a lot simpler than "TcpListener::accept calls accept4(2) and then tries to patch other bits of the socket up". CLOEXEC is the ~one exception to this rule since it's so important.

[hashmap]

Agree reg race, I found unfortunate that set_nonblocking and friends don’t take a mutable reference, while it doesn’t mutate any state in rust code it does system wide.

It also doesn’t allow to introduce any state to avoid extra syscall, however that design may be error prone. I think for non-blocking io in the majority of the cases we would have the same amount of syscalls, while doubling it for blocking io.

I’m not sure about the last point. I think about complexity from api user point of view. How many people needs to know details of impl and how hard to figure it out? At the same time there are millions of users of this method and for them we have a complex story - “what you get depends on listener state unless you are on linux then you always get blocking stream”. Also there is no single hint that this call is platform-dependent.

[Dylan-DPC-zz]

r? @KodrAus

[bors]

☔ The latest upstream changes (presumably #67449) made this pull request unmergeable. Please resolve the merge conflicts.

[JohnCSimon]

Ping from triage: @hashmap can you please address the merge conflicts?

[JohnCSimon]

Pinging again from triage: @hashmap can you post the status of this pr? Thanks.

[hashmap]

Sorry, I've finally rebased this pr.

[rust-highfive]

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2020-01-28T17:03:32.6223666Z ========================== Starting Command Output ===========================
2020-01-28T17:03:32.6226175Z [command]/bin/bash --noprofile --norc /home/vsts/work/_temp/b6fd0137-099f-4535-9c77-56bff62c9f91.sh
2020-01-28T17:03:32.6226215Z 
2020-01-28T17:03:32.6229001Z ##[section]Finishing: Disable git automatic line ending conversion
2020-01-28T17:03:32.6234209Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/67028/merge to s
2020-01-28T17:03:32.6235792Z Task         : Get sources
2020-01-28T17:03:32.6235826Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-01-28T17:03:32.6235897Z Version      : 1.0.0
2020-01-28T17:03:32.6235931Z Author       : Microsoft
---
2020-01-28T17:03:33.6917398Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-01-28T17:03:33.6929172Z ##[command]git config gc.auto 0
2020-01-28T17:03:33.6932358Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-01-28T17:03:33.6934298Z ##[command]git config --get-all http.proxy
2020-01-28T17:03:33.6940951Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/67028/merge:refs/remotes/pull/67028/merge

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

[sfackler]

I'm still not convinced that the standard library should be trying to fudge this.

@rfcbot fcp close

[rfcbot]

Team member @sfackler has proposed to close this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @KodrAus
• @SimonSapin
• @alexcrichton
• @dtolnay
• @sfackler
• @withoutboats

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[RalfJung]

Also there is no single hint that this call is platform-dependent.

Adding documentation to explain this better would probably makes sense, if the decision is to not change behavior.

[rfcbot]

The final comment period, with a disposition to close, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.