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

Set SO_REUSEADDR by default in libnative. #11845

Merged
merged 1 commit into from
Jan 28, 2014
Merged

Conversation

xales
Copy link
Contributor

@xales xales commented Jan 27, 2014

Fixes std::net test error when re-running too quickly.

Suggested by @cmr

@emberian
Copy link
Member

This should also fix the rust-http libnative thingy. cc @alexcrichton

@alexcrichton
Copy link
Member

According to this description I'd be wary of setting this option on windows as well. It looks like SO_REUSEADDR on windows essentially steals ports from any other application, which seems bad.

Otherwise, my current understanding of SO_REUSEADDR for tcp sockets is such that this is a good thing. Taking notes out of libuv's book, it looks like we don't need to do much on windows at all, so I think it's fine to wrap this in a if !cfg!(windows) or something like that.

Additionally, I don't think that we should ignore the error from the setsockopt call, it should be processed like the other errors.

Thanks for doing this!

@@ -96,7 +96,10 @@ fn socket(addr: ip::SocketAddr, ty: libc::c_int) -> IoResult<sock_t> {
};
match libc::socket(fam, ty, 0) {
-1 => Err(super::last_error()),
fd => Ok(fd),
fd => {
setsockopt(fd, libc::SOL_SOCKET, libc::SO_REUSEADDR, 1 as libc::c_int);
Copy link
Member

Choose a reason for hiding this comment

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

This code path is executed by both clients (connect) and servers (bind), but I think that we only want this as part of bind, could you move the logic into the bind function of the TcpListener?

Fixes std::net test error when re-running too quickly.
@vadimcn
Copy link
Contributor

vadimcn commented Jan 28, 2014

I thought that the proper way of avoiding "address already in use" error is to disconnect gracefully:

  • call shutdown(socket, 2)
  • read(socket, ...) till it returns zero
  • only then close(socket)

See e.g. http://hea-www.harvard.edu/~fine/Tech/addrinuse.html

@emberian
Copy link
Member

This doesn't help for rust-http or some other use cases where the process
is killed, rather than given a chance to shutdown.

On Tue, Jan 28, 2014 at 3:28 AM, Vadim Chugunov notifications@github.comwrote:

I thought that the proper way of avoiding "address already in use" error
is to disconnect gracefully:

  • call shutdown(socket, 2)
  • read(socket, ...) till it returns zero
  • only then close(socket)

See e.g. http://hea-www.harvard.edu/~fine/Tech/addrinuse.html


Reply to this email directly or view it on GitHubhttps://github.com//pull/11845#issuecomment-33459167
.

@vadimcn
Copy link
Contributor

vadimcn commented Jan 28, 2014

This may be appropriate for some apps, but just setting it by default seems... a bit dubious.
Also, I guess RtioSocket needs a shutdown() method?

@alexcrichton
Copy link
Member

For now I'd like to explore this route rather than using shutdown() manually. Using libuv already does this for us, and I don't think that this has many adverse effects (just a guess though).

Also, @xales, feel free to comment on PRs when you force-push update them. Sadly github doesn't send out any notifications on a force-push.

bors added a commit that referenced this pull request Jan 28, 2014
Fixes std::net test error when re-running too quickly.

Suggested by @cmr
@bors bors closed this Jan 28, 2014
@bors bors merged commit e901c4c into rust-lang:master Jan 28, 2014
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 1, 2023
…ion-extension, r=flip1995

Extend `result_map_or_into_option` lint to handle `Result::map_or_else(|_| None, Some)`

Fixes rust-lang/rust-clippy#10365.

As indicated in the title, it extends the `result_map_or_into_option` lint to handle `Result::map_or_else(|_| None, Some)`.

changelog: extension of the `result_map_or_into_option` lint to handle `Result::map_or_else(|_| None, Some)`

r? `@blyxyas`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants