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: add (*File).SetReadDeadline and (*File).SetWriteDeadline #22114

Closed
ianlancetaylor opened this issue Oct 2, 2017 · 13 comments
Closed

os: add (*File).SetReadDeadline and (*File).SetWriteDeadline #22114

ianlancetaylor opened this issue Oct 2, 2017 · 13 comments

Comments

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 2, 2017

Now that the os package uses the poller for some cases, we should add three new methods to the os.File type:

SetDeadline(t time.Time) error
SetReadDeadline(t time.Time) error
SetWriteDeadline(t time.Time) error

These methods are intended to act exactly like the similar methods on the various types in the net package that implement the net.Conn interface.

This will permit programs to set deadlines on pipes, TTYs, and perhaps other kinds of I/O objects most naturally handled via the os package.

This proposal replaces the earlier proposals #21834 and #22100, which had more complex contexts that need not be considered in the consideration of this proposal.

@tv42

This comment has been minimized.

Copy link

@tv42 tv42 commented Oct 2, 2017

Are there plans to support context for file operations, specifically cancellation?

Use case: FUSE filesystem server, implemented using underlying files which may be remote (as in NFS etc) or just slow (disk spun down or IO errors), kernel can cancel FUSE requests at any time, including based on end user control-C.

Use case 2: HTTP static file serving, similar scenarios.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor Author

@ianlancetaylor ianlancetaylor commented Oct 3, 2017

@tv42 I'm certainly open to it but I don't understand how to implement it efficiently.

@tv42

This comment has been minimized.

Copy link

@tv42 tv42 commented Oct 3, 2017

@ianlancetaylor Heh, I hoped it would have somehow followed from deadlines.

@juliandroid

This comment has been minimized.

Copy link

@juliandroid juliandroid commented Oct 3, 2017

Not sure, whether this is the right place to ask (I've mentioned that idea in #22100), but since we have already internal/poll, is it possible to also have (in addition to the Deadline methods above) a io.File method that has prototype similar to:

Async() <-chan int

This method will make read/write on io.File non-blocking and returns a notification channel (int type could be some concrete type of io package).

Where the channel is used to receive notifications, like: FD is ready to read/is closed (or just close(channel)).

That need comes naturally, if you look at my current workaround with sigio approach (and syscalls): https://play.golang.org/p/k5R8RcMPwg

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor Author

@ianlancetaylor ianlancetaylor commented Oct 3, 2017

@juliandroid From your code it looks like you basically want an interruptible read. You can get that using the SetReadDeadline in this proposal. Combine that with a goroutine doing the read and sending the results on a channel, and I think you have everything you want.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor Author

@ianlancetaylor ianlancetaylor commented Oct 3, 2017

Set a deadline of one minute and start a goroutine that reads from the TTY and sends the data over the channel. Handle your small timeout in a goroutine that reads from a timer channel and from the TTY read channel.

Yes, your Async suggestion can be useful, but it requires a lot of mechanism, it will be used very very rarely, and the operations that it supports can be done without it.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor Author

@ianlancetaylor ianlancetaylor commented Oct 3, 2017

In any case this proposal is about the deadline methods.

@juliandroid

This comment has been minimized.

Copy link

@juliandroid juliandroid commented Oct 3, 2017

I think you are mostly correct, although in some cases, I might be more interested in performance then I have to set bigger timeout and then lose flexibility to terminate the goroutine quickly.
If I set the timeout to some very small value then I have to do a lot of read calls unnecessary.
Both combined seems best approach.

One way to mitigate the issue is by passing second optional argument to the Set(Read)Deadline.
I.e. to have one argument with bigger timeout value that is the maximum timeout for whole read to complete and one argument that is a SoftTimeout that will interrupt the Read() operation only if the read buffer is empty. In other words, If there are still bytes coming, in less than SoftTimeout ms then Read() continues to deliver bytes. Every time you get data in less than SoftTimeout you have again another SoftTimeout for the next batch of bytes until EOF, close() or the global timeout expires.

Maybe it is a bit too complicated, although logical and maybe faster :)

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor Author

@ianlancetaylor ianlancetaylor commented Oct 3, 2017

What you are looking for is the termios VMIN and VTIME values.

Let's keep this issue about the proposed deadline methods.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Oct 16, 2017

SGTM. I'm assuming they will return a non-nil error for non-pollable files.

@ianlancetaylor ianlancetaylor changed the title proposal: os: add (*File).SetReadDeadline and (*File).SetWriteDeadline os: add (*File).SetReadDeadline and (*File).SetWriteDeadline Oct 16, 2017
@ianlancetaylor ianlancetaylor self-assigned this Oct 16, 2017
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 18, 2017

Change https://golang.org/cl/71751 mentions this issue: internal/poll: always decref if setting deadline fails

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 18, 2017

Change https://golang.org/cl/71770 mentions this issue: os: add deadline methods for File type

gopherbot pushed a commit that referenced this issue Oct 19, 2017
No test because at present it is never called in a way that fails.
When #22114 is implemented, failure will be possible. Not including this
change in that work because this change is separable and clearly correct.

Updates #22114

Change-Id: I81eb9eec8800e8082d918c0e5fb71282f538267e
Reviewed-on: https://go-review.googlesource.com/71751
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Joe Tsai <joetsai@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 19, 2017

Change https://golang.org/cl/71973 mentions this issue: runtime: for kqueue treat EVFILT_READ with EV_EOF as permitting a write

gopherbot pushed a commit that referenced this issue Oct 20, 2017
On systems that use kqueue, we always register descriptors for both
EVFILT_READ and EVFILT_WRITE. On at least FreeBSD and OpenBSD, when
the write end of a pipe is registered for EVFILT_READ and EVFILT_WRITE
events, and the read end of the pipe is closed, kqueue reports an
EVFILT_READ event with EV_EOF set, but does not report an EVFILT_WRITE
event. Since the write to the pipe is waiting for an EVFILT_WRITE
event, closing the read end of a pipe can cause the write end to hang
rather than attempt another write which will fail with EPIPE.

Fix this by treating EVFILT_READ with EV_EOF set as making both reads
and writes ready to proceed.

The real test for this is in CL 71770, which tests using various
timeouts with pipes.

Updates #22114

Change-Id: Ib23fbaaddbccd8eee77bdf18f27a7f0aa50e2742
Reviewed-on: https://go-review.googlesource.com/71973
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@gopherbot gopherbot closed this in 187957d Oct 25, 2017
@golang golang locked and limited conversation to collaborators Oct 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.