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.

Sign up for GitHub

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

doc: clarify effect of stream.destroy() on write() #25973

Closed
wants to merge 1 commit into from
+7 −2
Closed

doc: clarify effect of stream.destroy() on write() #25973

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions doc/api/stream.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,12 +369,17 @@ See also: [`writable.uncork()`][].
added: v8.0.0
-->

* `error` {Error}
* `error` {Error} Optional, an error to emit with `'error'` event.
Copy link
Member

@Trott Trott Feb 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `error` {Error} Optional, an error to emit with `'error'` event.
* `error` {Error} Optional, an error used as an argument for any callbacks
listening to the `'error'` event.

...or something like that? Nit-picky perhaps, but errors aren't emitted; events are. Anyway, optional nit, feel free to ignore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought I said that, and I don't want to reword the EventEmitter.emit() docs here. How about I reorder the words: "Optional, the 'error' event will be emitted with the error as an argument."?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems OK to me.

Copy link
Member

@Trott Trott Feb 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Although "as an argument for any listeners" might be a hair better? Anyway, I'm OK with any of the possibilities here. Improvements/clarifications can always come at a later date.)

* Returns: {this}

Destroy the stream, and emit the passed `'error'` and a `'close'` event.
Destroy the stream. Optionally emit an `'error'` event, and always emit
a `'close'` event.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This chunk and the previous one are obvious doc clarifications, its the chunk after this that I'm not sure about.

After this call, the writable stream has ended and subsequent calls
to `write()` or `end()` will result in an `ERR_STREAM_DESTROYED` error.
This is a destructive and immediate way to destroy a stream. Previous calls to
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above sentence defines interaction with subsequent calls, but interaction with previous calls was undefined.

`write()` may not have drained, and may trigger an `ERR_STREAM_DESTROYED` error.
Copy link
Member

@addaleax addaleax Feb 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think #24062 should have prevented exactly this :(

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(i.e. if this does still happen, I’d say it’s a bug.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's too bad, I hoped I could rewrite the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be removed from the docs then? Its clearly possible, at least in one specific instance, but that doesn't mean people should code to expect it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sam-github I am not eager to document it if it’s buggy, yes …

If you point me in the direction of steps to reproduce, I’ll definitely look into it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax In the process of preparing a decent branch for you to look at I realized I was causing this problem myself, trying to cause data to flow in a way that fixed some things, but broke others. I'm still making progress, I think I just fixed the problem causing data not to flow with tls1.3 (the right way, this time), but if I run into another streams related wall, I'll take you up on this offer.

Wrt to this specific PR, I'll remove the reference to things that shouldn't happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove the ref in a follow up PR.

Use `end()` instead of destroy if data should flush before close, or wait for
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Feb 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

destroy -> `destroy()` (with rewrapping, unfortunately).

the `'drain'` event before destroying the stream.
Implementors should not override this method,
but instead implement [`writable._destroy()`][writable-_destroy].

Expand Down