mdlayher / vsock

Package vsock provides access to Linux VM sockets (AF_VSOCK) for communication between a hypervisor and its virtual machines. MIT Licensed.
MIT License
336 stars 65 forks source link

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

Closed damz closed 6 years ago

damz commented 6 years ago

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@ea5825b0b64e1a017a76eac0ad734e11ff557c8e) 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 commented 6 years ago

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

damz commented 6 years ago

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

mdlayher commented 6 years ago

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 commented 6 years ago

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

mdlayher commented 6 years ago

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.