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

Conn.Close() doesn't unblock pending Read operations #13

Closed
damz opened this issue Apr 19, 2018 · 5 comments
Closed

Conn.Close() doesn't unblock pending Read operations #13

damz opened this issue Apr 19, 2018 · 5 comments

Comments

@damz
Copy link

damz commented Apr 19, 2018

This is a pretty big and unexpected departure from the net.Conn contract, which explicitly states:

// Close closes the connection.
// Any blocked Read or Write operations will be unblocked and return errors.
Close() error

The reason it happens is that in current versions of Go, os.NewFile() assumes that the provided file descriptor points to a file and doesn't support polling. As a consequence, it uses straight syscalls to handle Read and Write, and it is a well known weirdness of Linux that close() doesn't unblock recv*() from other threads. From the close(2) manpage:

When dealing with sockets, you have to be sure that there is no recv(2) still blocking on it on another
thread, otherwise it might block forever, since no more messages will be send via the socket. Be sure to use shutdown(2) to shut down all parts the connection before closing the socket.

From there, there are two options:

  1. Extend the current Close() to call shutdown(), which will unblock the blocked recv();
  2. Wait for Go 1.11, which now has support (since golang/go@ea5825b) for creating pollable os.File objects from os.NewFile(). For that, all you have to do is to set the file descriptor to non-blocking mode before passing it to os.NewFile()
@damz
Copy link
Author

damz commented Apr 19, 2018

(The advantage for waiting for Go 1.11 is that deadlines will work out of the box too.)

@damz
Copy link
Author

damz commented Apr 19, 2018

I implemented the Go 1.11+ option in a3be7e1, and it works beautifully.

@mdlayher
Copy link
Owner

Very interesting. So if I understand correctly we will get timeouts and everything for free with Go 1.11 too? That's been one of my biggest gripes with nonstandard socket types for years.

Once 1.11 lands I have no qualms making changes and requiring 1.11 to build this package. In the mean time, it'd probably be good to do the workaround you suggested for 1.10 and below.

@damz
Copy link
Author

damz commented Apr 20, 2018

@mdlayher Yes, with Go 1.11 and the patch in a3be7e1 you get for free:

  • Proper handling of these sockets in the network poller, instead of sacrificing a thread for each blocking operation;
  • The Close unblocks Read and Write semantics;
  • Timeouts (SetDeadline, SetReadDeadline and SetWriteDeadline).

@mdlayher
Copy link
Owner

Thanks again for the report. We hit this issue in production and I made a comparable patch to solve the issue. Closing this issue now as it appears to be fixed.

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

No branches or pull requests

2 participants