Skip to content

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

Rodrigo Muino Tomonari requested to merge github/fork/sam-github/doc-stream-destroy-side-effects into master

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

https://github.com/nodejs/node/blob/a046ae5ceddcaaf695df60be5dbc9d725beb6f22/test/parallel/test-tls-destroy-stream.js#L26-L27

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

https://github.com/nodejs/node/blob/a046ae5ceddcaaf695df60be5dbc9d725beb6f22/test/parallel/test-tls-invoke-queued.js#L39-L44

In this case, the destroy() doesn't error... but the 'end' event on the client happens before ANY 'data' events occur ... received is '' at https://github.com/nodejs/node/blob/a046ae5ceddcaaf695df60be5dbc9d725beb6f22/test/parallel/test-tls-invoke-queued.js#L56

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

Merge request reports