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

proposal: os: (*File).SetReadDeadline and (*File).SetWriteDeadline #21834

Closed
ccbrown opened this issue Sep 11, 2017 · 11 comments
Closed

proposal: os: (*File).SetReadDeadline and (*File).SetWriteDeadline #21834

ccbrown opened this issue Sep 11, 2017 · 11 comments

Comments

@ccbrown
Copy link

@ccbrown ccbrown commented Sep 11, 2017

Proposal

Add three new methods to os.File:

// SetDeadline sets the read and write deadlines associated
// with the connection. It is equivalent to calling both
// SetReadDeadline and SetWriteDeadline.
//
// A deadline is an absolute time after which I/O operations
// fail with a timeout (see type Error) instead of
// blocking. The deadline applies to all future and pending
// I/O, not just the immediately following call to Read or
// Write. After a deadline has been exceeded, the connection
// can be refreshed by setting a deadline in the future.
//
// An idle timeout can be implemented by repeatedly extending
// the deadline after successful Read or Write calls.
//
// A zero value for t means I/O operations will not time out.
SetDeadline(t time.Time) error

// SetReadDeadline sets the deadline for future Read calls
// and any currently-blocked Read call.
// A zero value for t means Read will not time out.
SetReadDeadline(t time.Time) error

// SetWriteDeadline sets the deadline for future Write calls
// and any currently-blocked Write call.
// Even if write times out, it may return n > 0, indicating that
// some of the data was successfully written.
// A zero value for t means Write will not time out.
SetWriteDeadline(t time.Time) error

If these look familiar, it's because I've copied and pasted them from the net package's Conn interface.

In Go 1.9, the os package now uses the internal runtime poller for file IO where possible (release note). Internally, there is already good support for timing out reads and writes on pollable files (The poll file descriptor already has these methods.). So the amount of engineering effort required to implement this proposal should be relatively minimal for those. Files that are not pollable can just return an error if you try to set a deadline (in the same manner as the net package's pipes do). See also:

An Alternative Proposal

Add two new methods to os.File:

ReadContext(ctx context.Context, b []byte) (n int, err error)
WriteContext(ctx context.Context, b []byte) (n int, err error)

Again, these would be only supported by pollable files, and would immediately return an error for all other files.

@gopherbot gopherbot added this to the Proposal milestone Sep 11, 2017
@gopherbot gopherbot added the Proposal label Sep 11, 2017
@davecheney

This comment has been minimized.

Copy link
Contributor

@davecheney davecheney commented Sep 11, 2017

@ccbrown

This comment has been minimized.

Copy link
Author

@ccbrown ccbrown commented Sep 11, 2017

@davecheney It would give you a safe alternative to calling Close concurrently. Instead, you would wait until your Read or Write is unblocked by a timeout (or context cancellation in the case of the alternative proposal), then Close the file.

@davecheney

This comment has been minimized.

Copy link
Contributor

@davecheney davecheney commented Sep 11, 2017

@dsnet

This comment has been minimized.

Copy link
Member

@dsnet dsnet commented Sep 11, 2017

Related to #20280.

\cc @zombiezen

@ccbrown

This comment has been minimized.

Copy link
Author

@ccbrown ccbrown commented Sep 11, 2017

I don't see how this proposal closes the read/close fd reuse hole. I would
prefer that assertion was removed from this proposal. Thanks.

@davecheney I'll gladly remove any incorrect or irrelevant assertions from my proposal. But I'm not sure what you're asking me to remove. I'm definitely not asserting that this proposal closes the file descriptor re-use hole. The context about the file descriptors is there because it reinforces the premise that I'm using to justify the proposal:

Aborting or timing out reads and writes is something that people both want to do and have a lot of trouble doing correctly.

The first set of links is to help illustrate the need: This proposal would provide an elegant solution to all of those posters. The second set of links is to help illustrate that it's currently very difficult to accomplish this safely: Implementing your own solution via syscall isn't portable or well-supported at all.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 13, 2017

In Go 1.9 and later the os package will protect against calling Close in one goroutine while there is a Read or Write occurring in another goroutine. The file descriptor will not be closed until the Read or Write completes.

This proposal is still meaningful but I believe the background section is simply wrong. I suggest editing the proposal to drop the background section, or closing this one and starting over. Sorry.

@ccbrown

This comment has been minimized.

Copy link
Author

@ccbrown ccbrown commented Sep 13, 2017

@ianlancetaylor I'm aware of the 1.9 behavior, but that doesn't invalidate my points. Even if the 1.9 implementation for some platforms behaves well, it's definitely not "well-supported" or "safe" (words that I was very careful to qualify my statements with) to rely on this implementation detail.

I think the background section is far from "simply wrong" and is extremely relevant, but I'll remove it from the proposal since for whatever reason it seems to be a distraction. People can draw their own conclusions about why os.File cancellation or timeouts may or may not be useful.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 13, 2017

@ccbrown My point was that in 1.9 calling Close in one goroutine while another goroutine is calling Read or Write is indeed safe. It is not an implementation detail. Before 1.9 it was a bug that this case was broken (#7970), and now that bug is fixed.

@ccbrown

This comment has been minimized.

Copy link
Author

@ccbrown ccbrown commented Sep 13, 2017

@ianlancetaylor I see. If it's the case that the behavior of Close during Read or Write is well-defined and safe on all platforms, I'm gonna close this issue. That satisfies my use-case as well as the use-cases I previously referenced. Thanks for your time!

@ccbrown ccbrown closed this Sep 13, 2017
@juliandroid

This comment has been minimized.

Copy link

@juliandroid juliandroid commented Sep 29, 2017

Basically, we don't have solution for non-blocking reads (os.File / Read()) and the only way to exit a goroutine is by closing the file (which is needed, in my case, at other places/goroutines and is not acceptable to be closed)?

I guess the current workaround is to use syscall.O_ASYNC|syscall.O_NONBLOCK and syscall.Read

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 29, 2017

@juliandroid This issue is closed, as it was more or less focused on safely using concurrent Read/Write and Close. If you want to open a new issue, please do that. If you want to discuss the matter, please use golang-dev. Thanks.

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
6 participants
You can’t perform that action at this time.