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

sam-github · 2019-02-06T23:55:41Z

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

node/test/parallel/test-tls-destroy-stream.js

Lines 26 to 27 in a046ae5

    
           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'));                                      
});

node/test/parallel/test-tls-invoke-queued.js

Lines 39 to 44 in a046ae5

    
           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

node/test/parallel/test-tls-invoke-queued.js

Line 56 in a046ae5

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

nodejs-github-bot · 2019-02-06T23:55:43Z

@sam-github build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2531/pipeline

sam-github · 2019-02-06T23:58:24Z

doc/api/stream.md

 * 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 chunk and the previous one are obvious doc clarifications, its the chunk after this that I'm not sure about.

sam-github · 2019-02-06T23:59:17Z

doc/api/stream.md

 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


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

addaleax · 2019-02-07T00:01:34Z

doc/api/stream.md

 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.


I think #24062 should have prevented exactly this :(

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

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

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.

@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

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

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

vsemozhetbyt · 2019-02-07T01:48:34Z

doc/api/stream.md

 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


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

mcollina

LGTM

Trott · 2019-02-07T03:00:23Z

doc/api/stream.md

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

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


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.

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

That seems OK to me.

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

danbev · 2019-02-14T03:58:05Z

Landed in 69a8e34.

PR-URL: nodejs#25973 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

sam-github · 2019-02-14T05:13:26Z

@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 · 2019-02-14T07:17:59Z

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

PR-URL: #25973 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

nodejs-github-bot added doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem. labels Feb 6, 2019

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

787ba1a

sam-github force-pushed the doc-stream-destroy-side-effects branch from c927a6b to 787ba1a Compare February 6, 2019 23:57

sam-github commented Feb 6, 2019

View reviewed changes

addaleax reviewed Feb 7, 2019

View reviewed changes

vsemozhetbyt reviewed Feb 7, 2019

View reviewed changes

mcollina approved these changes Feb 7, 2019

View reviewed changes

Trott reviewed Feb 7, 2019

View reviewed changes

sam-github mentioned this pull request Feb 9, 2019

TLS1.3 WIP sam-github/node#2

Closed

danbev approved these changes Feb 14, 2019

View reviewed changes

danbev closed this Feb 14, 2019

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

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

69a8e34

PR-URL: nodejs#25973 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

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

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

44fc2f6

PR-URL: #25973 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

targos mentioned this pull request Feb 14, 2019

v11.10.0 proposal #26098

Merged

sam-github deleted the doc-stream-destroy-side-effects branch February 21, 2019 18:50

quixote15 mentioned this pull request Sep 12, 2023

Writable does not check if stream has been destroyed during _final and _write #39030

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sam-github commented Feb 6, 2019

nodejs-github-bot commented Feb 6, 2019

sam-github Feb 6, 2019

sam-github Feb 6, 2019

addaleax Feb 7, 2019 •

edited

Loading

addaleax Feb 7, 2019

sam-github Feb 7, 2019

sam-github Feb 7, 2019

addaleax Feb 7, 2019

sam-github Feb 10, 2019

sam-github Feb 14, 2019

vsemozhetbyt Feb 7, 2019 •

edited

Loading

mcollina left a comment

Trott Feb 7, 2019 •

edited

Loading

sam-github Feb 7, 2019

Trott Feb 7, 2019

Trott Feb 7, 2019 •

edited

Loading

danbev commented Feb 14, 2019

sam-github commented Feb 14, 2019

danbev commented Feb 14, 2019

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

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

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

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

Conversation

sam-github commented Feb 6, 2019

Checklist

nodejs-github-bot commented Feb 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax Feb 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsemozhetbyt Feb 7, 2019 • edited Loading

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

Trott Feb 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott Feb 7, 2019 • edited Loading

Choose a reason for hiding this comment

danbev commented Feb 14, 2019

sam-github commented Feb 14, 2019

danbev commented Feb 14, 2019

addaleax Feb 7, 2019 •

edited

Loading

vsemozhetbyt Feb 7, 2019 •

edited

Loading

Trott Feb 7, 2019 •

edited

Loading

Trott Feb 7, 2019 •

edited

Loading