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

os: use non-blocking I/O for pollable files automatically #18507

Closed
elliotmr opened this issue Jan 3, 2017 · 41 comments
Closed

os: use non-blocking I/O for pollable files automatically #18507

elliotmr opened this issue Jan 3, 2017 · 41 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Proposal-Accepted
Milestone

Comments

@elliotmr
Copy link

elliotmr commented Jan 3, 2017

Issue

As Go matures as a language it is being adopted in areas beyond simple TCP/UDP servers and
command line software. Some people are working with non-standard socket types (AF_NETLINK,
AF_ROUTE 10565, AF_PACKET 15021, AF_CAN 16188, etc). Also, in the embedded area it
is very common to receive data through TTY devices or os pipes. All of these use cases
would benefit from as easy way to set timeouts without having to launch extra user-level
go routines or to cancel read/write operations without having to close the socket.

The current idiomatic way to deal with a timeout read on a generic file descriptor is as
follows:

fd, _ := syscall.Open(<path>, syscall.O_CLOEXEC, 0644)
buf := make([]byte, 1024)
success := make(chan bool)

go func() {
	_, err := syscall.Read(fd, buf)
	if err == nil {
		success <- true
	}
}()

select {
case <-success:
	fmt.Println(string(buf))
case <-time.After(5 * time.Second)
	syscall.Close(fd)  
	fmt.Println("timeout")
}

This has a few issues:

  1. You need to close the file descriptor, this is often not what you want to do
  2. The write to the success channel is sort of racy, it can be worked around with some
    clever channel work
  3. Since the read is a direct syscall, it not only blocks a go-routine, it blocks on
    os thread, and for embedded work with constrained resources this is something that
    should be avoided as much as possible

Proposal

The Go runtime has a single epoll/kqueue/iocp thread setup to deal with exactly these
issues for standard TCP/UDP/unix sockets. This runtime poller should be exposed for
generic file descriptors with a new package called "os/poll". The package would would
allow wrapping a file descriptor with a "net.Conn"-like interface.

The proposed API looks like this:

type Descriptor struct {
    // contains filtered or unexported fields
}

func Open(fd uintptr) (*Descriptor, error)
func (d *Descriptor) Close() error
func (d *Descriptor) Read(p []byte) (n int, err error)
func (d *Descriptor) Write(b []byte) (n int, err error)
func (d *Descriptor) SetDeadline(t time.Time) error
func (d *Descriptor) SetReadDeadline(t time.Time) error
func (d *Descriptor) SetWriteDeadline(t time.Time) error
func (d *Descriptor) Wait() error
func (d *Descriptor) WaitRead() error
func (d *Descriptor) WaitWrite() error
func (d *Descriptor) Cancel() error

This would provide a simple but flexible way to work with generic pollable file
descriptors. The example given above would then look like this:

fd, _ := syscall.Open(<path>, syscall.O_CLOEXEC, 0644)
buf := make([]byte, 1024)

desc, _ := poll.Open(fd)
desc.SetReadDeadline(time.Now().add(5 * time.Second)
_, err := desc.Read(buf)
if err != nil {
	fmt.Println("timeout")
} else {
	fmt.Println(string(buf)
}
@elliotmr
Copy link
Author

elliotmr commented Jan 3, 2017

Also, initial discussion on golang-dev.

@ianlancetaylor
Copy link
Member

Why provide Read and Write methods? Presumably people can continue to use the original descriptor.

Do we really need SetDeadline? Don't SetReadDeadline and SetWriteDeadline suffice?

Note that the current poller doesn't support Wait. It only supports WaitRead and WaitWrite.

Open and Close seem like the wrong names to me, even though that is what the current runtime code uses. It's more like Register and Unregister.

@rakyll rakyll added the Proposal label Jan 3, 2017
@elliotmr
Copy link
Author

elliotmr commented Jan 3, 2017

I put them in mainly for convenience, but you are correct that they aren't needed. You could construct Read, Write, Wait, and SetDeadline very easily from the other provided methods. My feeling that a polled read or polled write will be the use-case the majority of the time, so we might as well just put it in (it also means one less syscall function which I think is a good idea).

I also agree about Open and Close, I mentioned that in my comments in the golang-dev post, Register / Unregister or Deregister is more accurate. I will change those if that is the consensus.

@sbinet
Copy link
Member

sbinet commented Jan 3, 2017

Just a quick 'drive by' comment but perhaps the API should allow cancellation via 'context.Context'?

@elliotmr
Copy link
Author

elliotmr commented Jan 4, 2017

Adding a context could make the API simpler.

func Open(fd uintptr) (*Descriptor, error)
func (d *Descriptor) Close() error
func (d *Descriptor) Read(ctx context.Context, p []byte) (n int, err error)
func (d *Descriptor) Write(ctx context.Context, b []byte) (n int, err error)
func (d *Descriptor) WaitRead(ctx context.Context) error
func (d *Descriptor) WaitWrite(ctx context.Context) error

Maybe even removing the Read and Write methods, though I am not convinced that is a good idea.

@mdlayher
Copy link
Member

mdlayher commented Jan 4, 2017

Thanks a ton for this proposal @elliotmr . This is something I've been thinking about for quite a while as well.

If I had to guess, in my use-cases (raw sockets and netlink sockets), the wrapped Read and Write methods would probably not be as useful since I also want to be able to get at the syscall.Sockaddr implementations that are received and sent alongside the raw data.

A small API that simply enabled waiting for reads/writes to be ready, and the ability to cancel that wait using a context, would probably do the trick just fine for me.

@rakyll
Copy link
Contributor

rakyll commented Jan 4, 2017

Maybe we can put it under os package if the API surface is not big.

package os

func Poll(fd uintptr) (*PollHandle, error)
func (p *PollHandle) Close() error
func (p *PollHandle) WaitRead(ctx context.Context) error
func (p *PollHandle) WaitWrite(ctx context.Context) error

I think it is a bit weird to provide Read and Write via the poller, because the proposed APIs are working with the file descriptor. The call site already knows what the file descriptor represents, therefore how it should be consumed.

@rsc
Copy link
Contributor

rsc commented Jan 4, 2017

I don't see any reason for new API.

I think os.Files should be made to just work instead. You shouldn't have to go to extra effort for that. If you have an os.File that is poll-able, then it should use polling to avoid tying up a network thread when blocked, and maybe it should also expose timeouts.

Somewhere here there is an issue about making net.FileConn work for alternate sockets, by adding a registration function in package syscall, but I can't find it.

@rsc rsc added this to the Proposal milestone Jan 4, 2017
@elliotmr
Copy link
Author

elliotmr commented Jan 4, 2017

@rakyll I like the look of that, nice and simple but it gets the job done

@rsc I think that you are referring to 10565 which I linked above.

If you mean, no new API as in we could attach the new methods directly to a os.File object, then I could agree with that. @dvyukov suggested that in 6817. However, we still need the WaitRead and WaitWrite methods for the timeout/cancellation interface. Or at minimum new SetReadDeadline / SetWriteDeadline methods.

@mdlayher
Copy link
Member

mdlayher commented Jan 4, 2017

My previous proposal which was created after a discussion with @rsc is here: #15021.

It seems like this proposal could be the more "Go-like" interface that @bradfitz requested here: #15021 (comment).

@elliotmr
Copy link
Author

elliotmr commented Jan 4, 2017

So, maybe the best way would be to just add:

func (f *File) WaitRead(ctx context.Context) error
func (f *File) WaitWrite(ctx context.Context) error

to the os.File type. It would still require wiring the runtime poller to the os package, but it would allow for future refactoring of the os functionality to use non-blocking io.

The other option is that we could pursue #10565 or #15021, which would provide a more full-featured solution for alternative socket types, but not do anything for non-socket descriptors.

@dvyukov
Copy link
Member

dvyukov commented Jan 5, 2017

I might be missing something, but why do we need WaitRead/WaitWrite? Why can't we just make Read/Write non-blocking (in the same sense as net Read/Write)?

@elliotmr
Copy link
Author

elliotmr commented Jan 5, 2017

@dvyukov How do we set a timeout?

@dvyukov
Copy link
Member

dvyukov commented Jan 5, 2017

If we want Read with timeout, then we need to add Read with timeout. Doing it the same way as net.Conn sounds reasonable. My point was about Wait versions which look like a C interface.

@rsc
Copy link
Contributor

rsc commented Jan 5, 2017

You set a timeout the same way as in a net.Conn, with SetReadTimeout and SetWriteTimeout.

@elliotmr
Copy link
Author

elliotmr commented Jan 5, 2017

I am fine with that, so we would add SetReadDeadline and SetWriteDeadline to os.File, and make the functions non-blocking.

Essentially then the end user would only need to implement LocalAddr() and RemoteAddr() to their os.File to get a net.Conn

@mdlayher
Copy link
Member

mdlayher commented Jan 5, 2017

Making os.File nonblocking and adding deadline functions to os.File works for me.

In order to use a file descriptor opened via syscall.Socket or similar as an os.File, you have to use os.NewFile. So os.NewFile would also need to set the file descriptor to nonblocking and register it with the runtime poller, right?

@rsc
Copy link
Contributor

rsc commented Jan 5, 2017

Yes, it would have to be something like that.

@eklitzke
Copy link
Contributor

eklitzke commented Jan 6, 2017

I recently wrote a blog post about how the traditional select/epoll model makes it easier to pool buffers to use less memory: https://eklitzke.org/goroutines-nonblocking-io-and-memory-usage . It was pointed out to me that the problem I wrote about can be worked around in Go by using one-byte buffers, but actually being able to access the raw event loop would make it possible to do things even more efficiently.

What do you think of this use case? Admittedly it's a bit esoteric, but I do think there are situations when it's nice to know what file descriptors are readable/writeable before actually making the read/write system call.

@bradfitz
Copy link
Contributor

bradfitz commented Jan 6, 2017

@eklitzke, see also: #15735 (comment)

@eklitzke
Copy link
Contributor

eklitzke commented Jan 6, 2017

Wow, @bradfitz you are certainly prescient: the comment you linked to is exactly what I was describing. There are definitely a few different ways to solve this:

  • expose the event loop directly (makes things easy for people who know what they're doing, and hard for everyone else)
  • try to make the Go runtime do this kind of thing automatically (probably very diffcult? I imagine it's one of those "you kind of have to know up front" things)
  • something else?

I actually don't know what the answer is here, but I think this can be put into the "how to make C/C++ programmers consider Go more seriously" bucket.

@AudriusButkevicius
Copy link
Contributor

Yet I still feel that it'd be nice to have a case where net.Conn can be passed into selects, or where I can select on multiple connections at once, allowing all socket reads to come from the same routine, avoiding the need of locks.

@dvyukov
Copy link
Member

dvyukov commented Jan 7, 2017

Just for the sake of race-freedom, the following is not the idiomatic way to deal with timeouts. It's badly broken code with race on file descriptor. Such code can corrupt internal and external program state, up to including corrupting persistent state and leaking sensitive data to internet.

fd, _ := syscall.Open(<path>, syscall.O_CLOEXEC, 0644)
buf := make([]byte, 1024)
success := make(chan bool)

go func() {
	_, err := syscall.Read(fd, buf)
	if err == nil {
		success <- true
	}
}()

select {
case <-success:
	fmt.Println(string(buf))
case <-time.After(5 * time.Second)
	syscall.Close(fd)  
	fmt.Println("timeout")
}

@elliotmr

@elliotmr
Copy link
Author

elliotmr commented Jan 7, 2017

@dvyukov I agree with you, otherwise I wouldn't have raised this issue. I was just stating that it was suggested previously as the "Go" way to do it. I linked to some interesting discussions about it in the golang-dev post:

http://grokbase.com/t/gg/golang-nuts/14at84gaby/go-nuts-fd-polling-with-syscall-select
https://groups.google.com/forum/#!msg/golang-nuts/Wdpj20r360A/2gvP9q6hCwAJ
https://groups.google.com/forum/#!msg/golang-nuts/al-6_QFgYaM/UkCqyCQVq_0J

I haven't seen any other suggestions about how it should be done in Go, since resorting to using the syscall package is usually thought of as bad practice for application software.

@rsc
Copy link
Contributor

rsc commented Jan 9, 2017

Should we repurpose this proposal for making os.File "just work"? Specifically, make them not consume a thread when blocked (if possible, such as pipes, networks, and ttys) and also support deadlines (same set).

@mdlayher
Copy link
Member

mdlayher commented Jan 9, 2017

Should we repurpose this proposal for making os.File "just work"? Specifically, make them not consume a thread when blocked (if possible, such as pipes, networks, and ttys) and also support deadlines (same set).

I'm in favor. As long as I can set appropriate timeouts on the file and then use whatever syscall-ish functions I need to read data from an unsupported (not in stdlib) socket type, I'm happy.

Forgive my lack of knowledge in this area, but would this snippet work as expected if adding timeouts to *os.File is implemented? I'd like to still be able to use syscall and x/sys/unix functions for reading from sockets instead of just os.File.Read.

const family = 16
fd, _ := syscall.Socket(syscall.AF_NETLINK, syscall.SOCK_RAW, family)
// bind() and other setup logic

// Sets FD to nonblocking, add 2 second read timeout
f := os.NewFile(fd, "netlink")
_ = f.SetReadDeadline(time.Now().Add(2 * time.Second))

b := make([]byte, 128)
n, from, err := syscall.Recvfrom(fd, b, 0)
if err != nil {
    // Timeout after 2 seconds, or another error
}
// Work with n, from, and b

If this (or something similar) works with this proposed change, I'd be thrilled.

@AudriusButkevicius
Copy link
Contributor

AudriusButkevicius commented Jan 9, 2017

This does not cover the desire to multiple files/sockets/fds within the same routine via selects/poll etc, which is the case I am interested in.

Yes, I can use syscall to get access to the raw FDs etc, but you have libraries with return you connections or files which then you can't unwrap which you might like to select on etc. I think it'd be a low hanging fruit for the stdlib returned types which would implement a Pollable interface which could be passed to something like os/poll package.

Yet I might be confusing issues, as this topic is mostly about non-standard socket types, and not having an ability to timeout operations on them. If someone has ticket which talks about polling generic pipes/sockets/files, please point me to it.

@ianlancetaylor
Copy link
Member

@AudriusButkevicius I think you may be talking about something different than this proposal. This proposal is not about user code calling select or poll directly; it is about supporting arbitrary file descriptors in the single poller implemented in the runtime package. My apologies if I misunderstand.

gopherbot pushed a commit that referenced this issue Feb 13, 2017
This will make it possible to use the poller with the os package.

This is a lot of code movement but the behavior is intended to be
unchanged.

Update #6817.
Update #7903.
Update #15021.
Update #18507.

Change-Id: I1413685928017c32df5654ded73a2643820977ae
Reviewed-on: https://go-review.googlesource.com/36799
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Crawshaw <crawshaw@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
@rsc rsc changed the title proposal: os: use non-blocking I/O for pollable files automatically os: use non-blocking I/O for pollable files automatically Feb 13, 2017
@rsc rsc modified the milestones: Go1.9, Proposal Feb 13, 2017
@rsc
Copy link
Contributor

rsc commented Feb 13, 2017

As revised (make os do the right thing with no new API), accepted.

gopherbot pushed a commit that referenced this issue Feb 15, 2017
This changes the os package to use the runtime poller for file I/O
where possible. When a system call blocks on a pollable descriptor,
the goroutine will be blocked on the poller but the thread will be
released to run other goroutines. When using a non-pollable
descriptor, the os package will continue to use thread-blocking system
calls as before.

For example, on GNU/Linux, the runtime poller uses epoll. epoll does
not support ordinary disk files, so they will continue to use blocking
I/O as before. The poller will be used for pipes.

Since this means that the poller is used for many more programs, this
modifies the runtime to only block waiting for the poller if there is
some goroutine that is waiting on the poller. Otherwise, there is no
point, as the poller will never make any goroutine ready. This
preserves the runtime's current simple deadlock detection.

This seems to crash FreeBSD systems, so it is disabled on FreeBSD.
This is issue 19093.

Using the poller on Windows requires opening the file with
FILE_FLAG_OVERLAPPED. We should only do that if we can remove that
flag if the program calls the Fd method. This is issue 19098.

Update #6817.
Update #7903.
Update #15021.
Update #18507.
Update #19093.
Update #19098.

Change-Id: Ia5197dcefa7c6fbcca97d19a6f8621b2abcbb1fe
Reviewed-on: https://go-review.googlesource.com/36800
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
@yonderblue
Copy link

For those asking for deadlines above, wouldn't a more generic/backwards compatible solution to have a func (*os.File) SetContext(context.Context) be preferable? This would allow novel ways to do cancellation which the timeouts would not. Seems the internal polling would make this possible.

@tw4452852
Copy link
Contributor

tw4452852 commented May 24, 2017

While resolving this issue, I find that this change will filter EAGAIN error when (*os.File).Read on a nonblocking file which is our expected. For now, I have to use syscall.Read directly to work around this...

@ianlancetaylor
Copy link
Member

@tw4452852 Yes, the os package expects to handle I/O and polling for you in an efficient manner. If you want to manage non-blocking I/O yourself, then you should be using the syscall package, not the os package.

@tw4452852
Copy link
Contributor

@ianlancetaylor We'd better mention this on (os.File).Read document.

@bradfitz bradfitz modified the milestones: Go1.10, Go1.9 Jun 28, 2017
@bradfitz bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 28, 2017
@bradfitz
Copy link
Contributor

@ianlancetaylor, what's the status here? I didn't know so bumped it to Go 1.10, but maybe it's done? Or was this waiting on docs? It wasn't so labeled.

@ianlancetaylor
Copy link
Member

I didn't close it earlier because the os package still does not have SetReadDeadline or SetWriteDeadline methods. But I see now that we accepted this proposal with no new API. So now I'm not sure. We use the poller now, but without a way to set a deadline it doesn't seem fully functional to me.

@ccbrown
Copy link

ccbrown commented Sep 10, 2017

@ianlancetaylor Since this proposal was renamed / approved for an internal change only, maybe a separate issue should be opened for a cancellation and/or deadline API? I think the scope of this issue may be a bit muddled now.

@ianlancetaylor
Copy link
Member

@ccbrown Yes, I suppose someone should open a separate issue to add SetDeadline or use contexts for pollable os.File values. I will close this issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Proposal-Accepted
Projects
None yet
Development

No branches or pull requests