Skip to content

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

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

Closed
sam-github wants to merge 1 commit into from
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 numberDiff line numberDiff 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.

@TrottTrottFeb 7, 2019•
edited
Loading

Copy link
Copy Markdown
Member

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.

@sam-githubsam-githubFeb 7, 2019

Copy link
Copy Markdown
ContributorAuthor

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."?

@TrottTrottFeb 7, 2019

Copy link
Copy Markdown
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.

@TrottTrottFeb 7, 2019•
edited
Loading

Copy link
Copy Markdown
Member

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.

@sam-githubsam-githubFeb 6, 2019

Copy link
Copy Markdown
ContributorAuthor

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

@sam-githubsam-githubFeb 6, 2019

Copy link
Copy Markdown
ContributorAuthor

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.

@addaleaxaddaleaxFeb 7, 2019•
edited
Loading

Copy link
Copy Markdown
Member

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 :(

@addaleaxaddaleaxFeb 7, 2019

Copy link
Copy Markdown
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.)

@sam-githubsam-githubFeb 7, 2019

Copy link
Copy Markdown
ContributorAuthor

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.

@sam-githubsam-githubFeb 7, 2019

Copy link
Copy Markdown
ContributorAuthor

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.

@addaleaxaddaleaxFeb 7, 2019

Copy link
Copy Markdown
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

@sam-githubsam-githubFeb 10, 2019

Copy link
Copy Markdown
ContributorAuthor

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.

@sam-githubsam-githubFeb 14, 2019

Copy link
Copy Markdown
ContributorAuthor

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

@vsemozhetbytvsemozhetbytFeb 7, 2019•
edited
Loading

Copy link
Copy Markdown
Contributor

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