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

us.abort() is not called if us.write() fails #620

Open
ricea opened this issue Nov 25, 2016 · 6 comments
Open

us.abort() is not called if us.write() fails #620

ricea opened this issue Nov 25, 2016 · 6 comments
Milestone

Comments

@ricea
Copy link
Collaborator

ricea commented Nov 25, 2016

Currently an exception or rejection from us.write() will not result in us.abort() being called. If the underlying sink wants to free resources, it has to do it before returning the error. Something like this:

class UnderlyingSink {
  start() { this.handle = AcquireHandleSomehow(); }
  write(chunk) {
    return this.handle.write(chunk).catch(e -> {
      return this.close().then(() => throw e);
    });
  }
  close() {
    return this.handle.freeResources();
  }
}

If us.abort() (with fallback to us.close()) was called automatically, then this would become a lot simpler.

@ricea
Copy link
Collaborator Author

ricea commented Nov 25, 2016

I think we should also resolve the same question for controller.error() in this issue, as it is closely related. Currently controller.error() does not result in a call to us.abort().

@domenic
Copy link
Member

domenic commented Nov 26, 2016

I realized that there is a somewhat analogous situation for readable streams actually :-/. E.g. in https://streams.spec.whatwg.org/#example-rs-pull, if fs.read rejects, we do not call fs.close. Or if we call rsController.error(e). I am not sure what to think about that.

@ricea
Copy link
Collaborator Author

ricea commented Nov 28, 2016

My severity analysis:

Naively written sources and sinks will tend to leak resources. Some number of hours will be lost to debugging these resource leaks. Garbage collection will mitigate this issue for some Javascript-authored streams, but not all. For example, a WebSocket will stay alive as long as it is open and a message handler exists, so GC would not clean them up.

My usability analysis:

If finally() existed, it would be unambiguous what to do here. On the other hand, abort() or close() being called when there wasn't a corresponding abort() or close() at the reader/writer level might be considered confusing.

My proposal:

Do nothing now. Discuss adding finally() to sources and sinks as a "v2" feature.

@domenic
Copy link
Member

domenic commented Nov 29, 2016

The proposal sounds good. However I wonder if we should also remove the fallback from abort to close, to maintain the idea that the "v1" feature set is focused around obvious mappings, and not around convenient cleanup---especially convenient cleanup that only works in one of the many cases.

Also we should fix up all the examples to do proper cleanup.

@tyoshino
Copy link
Member

+1 for removal of the fallback

1 similar comment
@ricea
Copy link
Collaborator Author

ricea commented Nov 29, 2016

+1 for removal of the fallback

domenic added a commit that referenced this issue Dec 16, 2016
See discussion in #620 and in particular the conclusion at #620 (comment).
domenic added a commit that referenced this issue Jan 6, 2017
See discussion in #620 and in particular the conclusion at #620 (comment).
domenic added a commit that referenced this issue Jan 6, 2017
See discussion in #620 and in particular the conclusion at #620 (comment).
@domenic domenic modified the milestone: V2 Mar 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants