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: io/v2: add Context parameter to Reader, etc. #20280

Open
zombiezen opened this issue May 8, 2017 · 31 comments
Open

proposal: io/v2: add Context parameter to Reader, etc. #20280

zombiezen opened this issue May 8, 2017 · 31 comments
Labels
Proposal v2 An incompatible library change
Milestone

Comments

@zombiezen
Copy link
Contributor

zombiezen commented May 8, 2017

Related to #18507, it would be nice if the io interface methods all took in Context as their first parameter. This allows a cancellation signal to be attached directly to the I/O calls that are using them, instead of requiring an out-of-band method, as in net.Conn.

This is obviously a backward-incompatible change and would have broad impact on the ecosystem. I wanted to open this as a proposal for consideration in Go 2.

EDIT: See below for the more backward-compatible proposal

EDIT 2: I've written an experience report/blog post detailing the background for this proposal: Canceling I/O in Go Cap’n Proto. Feedback and alternatives welcome.

@zombiezen zombiezen added v2 An incompatible library change Proposal labels May 8, 2017
@gopherbot gopherbot added this to the Proposal milestone May 8, 2017
@dsnet
Copy link
Member

dsnet commented May 8, 2017

In addition to all of the Reader-like and Writer-like interfaces, we should also do this to io.Closer as close operations frequently involve one last set of write operations before closing the handle.

It's less obvious whether this should be done to io.Seeker (but perhaps we even remove that in Go2 #17920)? Oftentimes the implementation of io.Seeker is as simple as changing some file offset in the internal data structure, and shouldn't block.

@rasky
Copy link
Member

rasky commented May 8, 2017

Sorry to hijack, but if we get to this point, then Context should probably be rethought to be a feature of the language. The current io interfaces are very nice because they're very easy to work with yet very powerful and composable. Adding the complexity of correctly handling a context for all use cases seems overkill. If we want all goroutines to be correctly cancelable no matter what, then this is possibly better handled by the runtime.

@dsnet
Copy link
Member

dsnet commented May 8, 2017

@rasky, let's file a seperate issue for Context being part of the language.

Other than needing to do extra typing to add the argument for the context, there is not that much complexity. Compute-only Readers like compression doesn't even need to inspect the context. All it needs to do is plumb it through to the underlying io.Reader. This does not seem particularly complicated.

@rsc rsc changed the title proposal: add Context parameter to io interface types proposal: io: add Context parameter to Reader, etc. Jun 16, 2017
@dchapes
Copy link

dchapes commented Aug 7, 2017

It's real simple to make an io.Reader (or io.Writter) that wraps an existing io.Reader plus a context (including checking if the the context has a deadline and the reader implements SetReadDeadline). If anything, just add such wrappers to io/ioutil rather than mucking with the existing io.Reader and io.Writer interfaces.

@sirkon
Copy link

sirkon commented Aug 7, 2017

I don't bother be rude: the author is network monkey.
File IO on Linux is blocking. Imagine running another OS thread just to comply the functionality.

And I believe the problem of goroutine cancellation must be solved via the runtime tricks, because it only happened as a consequence of mismatch between synchronous representation of asynchronous environment, so let they introduce another tool to access the functionality that is out of scope now due to the mismatch.

@zombiezen
Copy link
Contributor Author

@dsnet and I came up with a way this could be introduced gradually. Instead of modifying the interfaces directly, taking the database/sql approach:

package ctxio // package "io/ctxio"

import (
  "context"
  "io"
)

type Reader interface {
  ReadCtx(context.Context, []byte) (int, error)
}

// BindReader returns an io.Reader that reads from r using the ctx Context.
// (Name is bikeshed-able.)
func BindReader(ctx context.Context, r Reader) io.Reader

// PromoteReader returns a Reader that reads from r.
// It will ignore any Context passed to the Read calls.
// (Name is bikeshed-able.)
func PromoteReader(r io.Reader) Reader

// and so on...

By having this be a different interface, existing concrete types (like *os.File) could implement both interfaces and this could be a backward-compatible change.

@dchapes: Yes, I have done this before. Where SetReadDeadline fails is if the Context has a manual cancel signal (perhaps triggered by SIGINT or some such), which at least in most code that I write, comes up more frequently. Readers and writers also end up being the hardest place to correctly propagate Contexts (especially values) for readers and writers that are shared amongst multiple contexts.

@sirkon raises a good point which would be the logical follow-on: it would be wonderful if *os.File and *net.TCPConn et al supported cancel signals at the scheduler level. Hand-waving, the only parts of Context that the scheduler would need to receive would be the Done() channel and the deadline, which could keep the runtime decoupled from the context package.

@sirkon
Copy link

sirkon commented Aug 7, 2017

@zombiezen

Try to read about blocking and non-blocking IO at least at the API level. TCPConn's socket is already non-blocking in Go and it is only needed to "just" add another coupled file descriptor (timer) into the event loop to control exit. "Just" because it is yet another syscall and syscalls are expensive.

On files: asynchronous file IO don't work well in Linux and turn to be blocking for heavy loads. So, you need to launch thread per file to guarantee asynchronous file reads/writes (+ scheduling thread, although it is already there).

These were just technical remarks.

And it, finally, the idea stinks. It just looks ugly, it is the shame you don't realize this yourself.
As I told, the issue itself is just a consequence of mismatch between asynchronous nature and its synchronous representation in Go. The best possible solution is on runtime level, e.g. goroutines with optional parameters.

@dsnet
Copy link
Member

dsnet commented Aug 7, 2017

@sirkon, thank you for your concerns about syscall efficiency. However, saying "the idea stinks. It just looks ugly, it is the shame you don't realize this yourself" doesn't help the discussion. I think you may be failing to recognize the problem that this issue is trying to solve. These issues are intended to be a place to discuss ideas in a civil manner.

The issue at hand is not just "asynchronous vs synchronous", but one with regards to plumbing context for cancelation signal, which (although often canceled asynchronously) and also value propagation are still different issues than "asynchronous vs synchronous" calling of Read. Plumbing Context through to all Readers and Writers is one such approach to addressing this issue, but carries with it obvious downsides of plumbing the context to many Reader and Writer implementations that do not care about it. There are probably many other approaches, and that's why we should discuss this.

If you have the "best possible solution" in mind, perhaps you would like to elaborate your ideas?

@sirkon
Copy link

sirkon commented Aug 8, 2017

I don't have any idea on the issue other than understanding this proposal is stupid.

By points:

  1. context.Context is poorly designed:
    1. there's cancelation task
    2. there's data storage (context) task
      These are different tasks . Stdlib designers made weird decisions before, like idiotic time format, so I cannot say I am amazed, but it is still embarassing to see such an ugly piece there.
  2. Some IO operations cannot be canceled at all, like file IO.

Back to the proposal:

  1. The person don't see the ugliness of context.Context
  2. The person don't understand why it is a bad idea to introduce concept into domain when not all items of it actually support it.

@zombiezen
Copy link
Contributor Author

@sirkon, I'm going to echo @dsnet's remarks here. Your responses are uncivil (I would like to point you to our Code of Conduct) and are commenting on the basis of "ugliness" rather than giving concrete technical objections.

I understand that an Experience Report will likely clear up some of the background on what problems this is trying to solve. I will try to write up something soon.

As for the objection to "introduce concept into domain when not all items of it actually support it", for the implementers of an io interface that don't require the Context, they can simply ignore the parameter, just as any function that takes in a Context that does not need it does. However, consider Readers and Writers that need a Context: there isn't as easy of a workaround. @dchapes's solution works in a number of cases involving deadlines, but it does not work in cases involving Context values.

@zombiezen
Copy link
Contributor Author

zombiezen commented Aug 8, 2017

For the record, this isn't a full-fledged proposal. I opened this issue since I feel this is something that should be considered for Go 2. It may be accepted; it may be rejected. The Go ecosystem is still finding its way on how and when to use Context. Networking and I/O are places where Context comes up frequently, so it's something that we may want to consider in the future. Once we get farther along and the problem space is more agreed upon, there may be better ways of achieving the same goals.

@zombiezen
Copy link
Contributor Author

zombiezen commented Jan 12, 2018

I've written an experience report/blog post detailing the background for this proposal: Canceling I/O in Go Cap’n Proto. Alternatives welcome.

@as
Copy link
Contributor

as commented Jan 13, 2018

I don't bother be rude: the author is network monkey.
File IO on Linux is blocking. Imagine running another OS thread just to comply the functionality.

Real network monkeys know that Go also runs on modern systems.

@ianlancetaylor
Copy link
Member

I think this proposal is asking for something deeper than changing io.Reader to use a Read method that takes a Context argument. I think this is asking for a way to interrupt a call to Read by canceling a Context.

As @dchapes says above, it is possible to build an io.Reader that incorporates a Context value and checks it before calling the underlying Read. That demonstrates that there is no clear need for adding a Context as an argument to every Read call. But that is (presumably) unsatisfactory because, with the wrapped Reader, if the Context is canceled, the Read will not be interrupted.

So I think we can set aside the idea of passing a Context to every Read method, and focus on the idea of whether it is possible to arrange for a Read to be interrupted, with an error, when some Context is canceled. We can implement that for descriptors that end up in the network poller; descriptors that do not end up there probably do not hang in Read in any case.

Can we devise a mechanism for associating a Context with a *os.File or net.Conn?

@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 13, 2018
@Julio-Guerra
Copy link

Julio-Guerra commented Feb 14, 2018

On POSIX systems, it would be great if Context could expose a file descriptor to signal its state changes. That way, we could use it with I/O event notification functions such as select(), poll(), etc. It removes one part of the problem: interfacing Context with file-operation functions.

The other part, unblocking a call, is very specific to the file descriptor and the function being used... So far I only had to unblock read() calls on both net.Conn and *os.File, and the same self-pipe technique implementation works.

@mjgarton
Copy link
Contributor

Associating a Context with a *os.File or net.Conn could be problematic because it applies to all ongoing operations. It is not uncommon for a read to be in progress on a network socket when a write comes along. I wouldn't want to cancel the write just because the read is being canceled (or vice versa)

I created this (incomplete) hack for testing my own purposes that may or may not be useful for reference.

Ignoring the implementation, the API at least served my purposes. https://gitlab.com/streamy/concon

@ianlancetaylor
Copy link
Member

@Julio-Guerra Context already exposes a channel for use in a select statement. We don't expect Go programmers to call the system calls select or poll themselves, since the runtime does that automatically anyhow. So I don't see a good reason for Context to expose a file descriptor. While there are cases where that could be useful, those cases seem very rare.

@zhixinwen
Copy link

@dchapes The proposal you made only works to some extent. For example, if we call func Reader with the same io.Reader several times, the read deadline can be infinitely extended, when the reader is slow.

Although this example is simple and can be avoided, I have seen more intricate cases with similar ideas which run into the same trouble.

mrnugget added a commit to sourcegraph/src-cli that referenced this issue Jun 23, 2022
This is essentially a copy-paste implementation of the ideas presented
in this comment here: golang/go#20280 (comment)

It fixes #775 and helps with the issue described in #793 (comment). Not sure if it has unintended side-effects.
BolajiOlajide pushed a commit to sourcegraph/src-cli that referenced this issue Jul 7, 2022
This is essentially a copy-paste implementation of the ideas presented
in this comment here: golang/go#20280 (comment)

It fixes #775 and helps with the issue described in #793 (comment). Not sure if it has unintended side-effects.
BolajiOlajide added a commit to sourcegraph/src-cli that referenced this issue Jul 7, 2022
* Cancel reading from stdin on Ctrl-C

This is essentially a copy-paste implementation of the ideas presented
in this comment here: golang/go#20280 (comment)

It fixes #775 and helps with the issue described in #793 (comment). Not sure if it has unintended side-effects.

* Add comments and remove interface

* update changelog

Co-authored-by: BolajiOlajide <25608335+BolajiOlajide@users.noreply.github.com>
scjohns pushed a commit to sourcegraph/src-cli that referenced this issue Apr 24, 2023
* Cancel reading from stdin on Ctrl-C

This is essentially a copy-paste implementation of the ideas presented
in this comment here: golang/go#20280 (comment)

It fixes #775 and helps with the issue described in #793 (comment). Not sure if it has unintended side-effects.

* Add comments and remove interface

* update changelog

Co-authored-by: BolajiOlajide <25608335+BolajiOlajide@users.noreply.github.com>
@ireina7
Copy link

ireina7 commented May 25, 2023

In golang, we design interfaces after structs. That means at the beginning, the real logic is not clear, we can use structs to implement it and finally use interface to reuse it in the future.

Therefore, whether io.Reader should have context parameter depends on the actual program logic and concurrency environment. If you want your logic always handle timeout or cancellation, you may use a different interface which has explicit context control.

In conclusion, I don't think we should redesign std interfaces. It's not necessary.

@purpleidea
Copy link

In lieu of having this feature upstream, and not seeing a complete proof of concept, I've implemented a cancellable password reader. Of course if a signal kills the process, this can leave your terminal in a bad state and you'd need to run reset but in most cases this is still what you want. It's here:

https://github.com/purpleidea/mgmt/tree/master/util/password

If anyone has improvements, please send a patch!

I'd comment in #24842 but it's locked.

HTH

@ireina7
Copy link

ireina7 commented Oct 12, 2023

What about a contexual interface?

type Contextual[T any] interface {
    Context(context.Context) T
}

type contextualReader struct {
    ctx context.Context
    reader io.Reader
}

func (r *contextualReader) Context(ctx context.Context) io.Reader {...}
func (r *contextualReader) Read(b []byte) (n int, err error) {...}

func logic(ctxReader Contextual[io.Reader]) {
    //...ctx...
    ctxReader.Context(ctx).Read(buf)
}

This interface force users to pass ctx before invoking any other methods.
With this abstraction, we can overpass many dependences like ctx. However, I don't think this is suitable for new interfaces, if you need to control context, just encode it into the interface method.

@debarshiray
Copy link

So I think we can set aside the idea of passing a Context to every Read method, and focus on the idea of whether it is possible to arrange for a Read to be interrupted, with an error, when some Context is canceled. We can implement that for descriptors that end up in the network poller; descriptors that do not end up there probably do not hang in Read in any case.

Can we devise a mechanism for associating a Context with a *os.File or net.Conn?

Maybe I am missing something, but this doesn't look that hard.

I recently wrote a bunch of asynchronous cancellable functions that read from a *os.File. The functions have a context.Context parameter, and wire up the done channel to an eventfd that's passed to poll(2) along with the file descriptor of the *os.File.

@ianlancetaylor ianlancetaylor changed the title proposal: io: add Context parameter to Reader, etc. proposal: io/v2: add Context parameter to Reader, etc. Aug 6, 2024
@ianlancetaylor ianlancetaylor removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Proposal v2 An incompatible library change
Projects
None yet
Development

No branches or pull requests