Read cannot be interrupted by Close

[nkovacs]

I was expecting Read to be interrupted by Close, but this doesn't work.

The issue is that in blocking mode, the read is not interrupted even if the file is closed until the vmin or vtime condition is satisfied, and because I use vmin > 0 and vtime > 0, Read would never return. The solution is to put it into non-blocking mode. In this case, read returns EAGAIN, and the go epoll implementation will properly wait for data to be available: https://github.com/golang/go/blob/release-branch.go1.12/src/internal/poll/fd_unix.go#L168

Just removing this isn't enough, because Fd puts it into blocking mode: https://github.com/golang/go/blob/release-branch.go1.12/src/os/file_unix.go#L70

So I removed the first SetNonblock call and added syscall.SetNonblock(int(file.Fd()), true) to the end of openInternal in open_linux.go. This works fine in my testing.

[LubosD]

Your suggestion unfortunately leads to the "resource temporarily unavailable" error when using bufio.ReadString('\n'), but in general, the fact that Read() cannot be interrupted is very bad.

[nkovacs]

That's the EAGAIN error which should be caught by poll, and it shouldn't matter if you're using bufio.ReadString or just plain Read: https://github.com/golang/go/blob/release-branch.go1.12/src/internal/poll/fd_unix.go#L168 What operating system are you using? Maybe the runtime failed to initialize polling on the file (https://github.com/golang/go/blob/release-branch.go1.12/src/os/file_unix.go#L156), but there's no easy way to detect that.

[LubosD]

@nkovacs The app I'm writing is for Linux/arm (Raspberry Pi).

I wasn't aware that Raspbian contains Go 1.7.4 (almost 3 years old), so I updated to Go 1.12.4 and there I get the expected behavior when I add syscall.SetNonblock(int(file.Fd()), true) into my own code. Thanks!

[nkovacs]

The poller was implemented for files in Go 1.9. One option is to use build tags and then use blocking mode in Go <1.9, but then Close won't work and will probably lead to a race condition as well. To make it work properly, you would have to wrap Read and Close, put a mutex in them, use non-blocking mode, vmin=0 and vtime>0, and if you get an EAGAIN, repeat the read (while letting Close interrupt it). This would waste cpu time of course, and Close would hang for up to vtime.

There's also the issue that we're assuming polling will always be available. If the poller fails to initialize, the file is not put into non-blocking mode (https://github.com/golang/go/blob/release-branch.go1.12/src/os/file_unix.go#L156), but since this package calls os.OpenFile with O_NONBLOCK, the file will be in non-blocking mode anyway, so there's no way to detect whether the poller init succeeded.

[chenwaichung]

@nkovacs Thank you for sharing the solution

[chmorgan]

I've forked at GitHub.com/chmorgan/go-serial2 btw

I'm seeing this issue on my test target. Should there be an option to be blocking vs. non-blocking? I'm not super familiar with the poller support. It seems super helpful to support non-blocking reads as well, we can't expect data to be returned all of the time.

[nkovacs]

From the perspective of your program, the read will still be blocking (with Go 1.9+), because the poller will handle all that (while the file is actually in non-blocking mode). That's the ideal situation, and there should be no reason to use blocking mode.

The main issue is that this package calls os.OpenFile with syscall.O_NONBLOCK, which unfortunately does two, separate things: it prevents the open from blocking for a long time and it sets the non-blocking flag. And because the non-blocking flag is set, this package cannot detect if Go's poller was successfully enabled for the file by checking if OpenFile itself set the flag.

You could call OpenFile without syscall.O_NONBLOCK, and let stdlib handle it, then check whether the non-blocking flag is set on the file (indicating the poller successfully initialized on the file), and restore it if needed after calling Fd (because Fd sets it to blocking mode). If the poller failed, then there's not much that can be done. You could implement a workaround like I described in my previous comment, or you could just deal with Read not being interruptible with Close (if you don't need to Close during the lifetime of your program, you won't get goroutine leaks, for example).

Unfortunately, calling OpenFile without syscall.O_NONBLOCK might lead to the open blocking for a long time, and I don't know under which circumstances it would happen. So another option is to always put the file into non-blocking mode (like my fork), and wrap Read/Write and handle EAGAIN yourself, in case the poller failed (Go <1.9 or unsupported architecture or whatever other reason).

[chmorgan]

@nkovacs you'd recommend I pull in https://github.com/nkovacs/go-serial/commit/a72614425108022f3aef1c6285ac0af5016d4761 and https://github.com/nkovacs/go-serial/commit/73fc76fa08e4e3f6dc7f5090745016b849522ac2 then?

In terms of usage should we update the example to make use of the poller? I'm not familiar with that module but it looks like great functionality to have when working with fds.

[chmorgan]

@nkovacs and I think its ok to assume Go >1.9 at this point, at least I'm not excited about adding support for such old versions at the moment. Someone else is welcome to though :-)

[nkovacs]

Yes, I think using non-blocking mode with the poller is the way to go. You can assume Go > 1.9, the issue is that the poller might not initialize in certain cases, but I don't know how common that is. I haven't had any problems on a Raspberry Pi.

[chmorgan]

Ahh very good!

I’ll merge those changes this afternoon. I’m collecting all of the good fixes to the library so they are in one maintained repo. If you have any other suggestions I’d love to hear them.

[chmorgan]

Thanks @nkovacs, changes merged into the go-serial2 fork, https://github.com/chmorgan/go-serial2