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

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

Closed

Conversation

Projects
None yet
7 participants
@sam-github
Copy link
Member

commented Feb 6, 2019

@mcollina or @addaleax or @nodejs/streams

I'd like some clarification on the expected interaction of destroy() and write(). I am going through my final rounds of test cleanups on my TLS1.3 WIP, and the remaining failures are mostly about stream interactions, and I'm not sure if they are bugs or invalid tests. The docs aren't clear on the expected behaviour, so I propose some text in this PR, but I'm not sure if its correct - though it is what I observe.

socket.write(CONTENT);
socket.destroy();

I'm getting ERR_STREAM_DESTROYED from the above. I will do detailed analysis tomorrow, but I suspect it is because after the secureConnection event, with TLS1.3, the SSL context is going to write some key update messages, so it slows down the flushing of CONTENT, and .destroy() causes the flushing to fail. Again, have to confirm that I understand what is happening, but either way, is the test valid? Note it passes if rewritten as:

socket.write(CONTENT, () => {                                                
   socket.destroy(new Error('hi, sam'));                                      
}); 

c.write('hello ', null, common.mustCall(function() {
c.write('world!', null, common.mustCall(function() {
c.destroy();
}));
// Data on next _write() will be written, and the cb will still be invoked
c.write(' gosh', null, common.mustCall());

In this case, the destroy() doesn't error... but the 'end' event on the client happens before ANY 'data' events occur ... received is '' at

assert.strictEqual(received, 'hello world! gosh');

Again, I have to do detailed analysis to see why the data was dropped, but this seems to me to be an invalid test case. Unlike end() which is guaranteed to be serialized with any preceeding write()s, I don't think destroy() has any such guarantee.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@sam-github sam-github force-pushed the sam-github:doc-stream-destroy-side-effects branch from c927a6b to 787ba1a Feb 6, 2019

* 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.

This comment has been minimized.

Copy link
@sam-github

sam-github Feb 6, 2019

Author Member

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

This comment has been minimized.

Copy link
@sam-github

sam-github Feb 6, 2019

Author Member

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

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
`write()` may not have drained, and may trigger an `ERR_STREAM_DESTROYED` error.

This comment has been minimized.

Copy link
@addaleax

addaleax Feb 7, 2019

Member

I think #24062 should have prevented exactly this :(

This comment has been minimized.

Copy link
@addaleax

addaleax Feb 7, 2019

Member

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

This comment has been minimized.

Copy link
@sam-github

sam-github Feb 7, 2019

Author Member

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

This comment has been minimized.

Copy link
@sam-github

sam-github Feb 7, 2019

Author Member

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.

This comment has been minimized.

Copy link
@addaleax

addaleax Feb 7, 2019

Member

@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

This comment has been minimized.

Copy link
@sam-github

sam-github Feb 10, 2019

Author Member

@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.

This comment has been minimized.

Copy link
@sam-github

sam-github Feb 14, 2019

Author Member

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

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
`write()` may not have drained, and may trigger an `ERR_STREAM_DESTROYED` error.
Use `end()` instead of destroy if data should flush before close, or wait for

This comment has been minimized.

Copy link
@vsemozhetbyt

vsemozhetbyt Feb 7, 2019

Contributor

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

@mcollina
Copy link
Member

left a comment

LGTM

@@ -369,12 +369,17 @@ See also: [`writable.uncork()`][].
added: v8.0.0
-->

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

This comment has been minimized.

Copy link
@Trott

Trott Feb 7, 2019

Member
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.

This comment has been minimized.

Copy link
@sam-github

sam-github Feb 7, 2019

Author Member

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

This comment has been minimized.

Copy link
@Trott

Trott Feb 7, 2019

Member

That seems OK to me.

This comment has been minimized.

Copy link
@Trott

Trott Feb 7, 2019

Member

(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.)

@sam-github sam-github referenced this pull request Feb 9, 2019

Closed

TLS1.3 WIP #2

@danbev

danbev approved these changes Feb 14, 2019

@danbev

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

Landed in 69a8e34.

@danbev danbev closed this Feb 14, 2019

pull bot pushed a commit to zys-contribs/node that referenced this pull request Feb 14, 2019

doc: clarify effect of stream.destroy() on write()
PR-URL: nodejs#25973
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@sam-github

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2019

@danbev I think that was too early, there were still outstanding comments requesting changes.

Unfortunately, they weren't made with the Request Changes features, so without reading all the comments in detail its not obvious.

I'll do a follow up PR to address the leftover comments.

@danbev

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

@sam-github Sorry about that and for causing the additional work.

targos added a commit that referenced this pull request Feb 14, 2019

doc: clarify effect of stream.destroy() on write()
PR-URL: #25973
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

@targos targos referenced this pull request Feb 14, 2019

Merged

v11.10.0 proposal #26098

@sam-github sam-github deleted the sam-github:doc-stream-destroy-side-effects branch Feb 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.