René's URL Explorer Experiment

{"@context":"https://schema.org","@type":"DiscussionForumPosting","headline":"Recent behavior change of buffered write callbacks","articleBody":"As requested by @mcollina in https://github.com/nodejs/node/pull/31179#issuecomment-572183520 I'm opening this issue to discuss the possibility of reverting a recent behavior change of writable streams.\r\n\r\n[writable.write()](https://nodejs.org/api/stream.html#stream_writable_write_chunk_encoding_callback) takes an optional callback which, quoting the documentation, is a\r\n\r\n\u003e Callback for when this chunk of data is flushed\r\n\r\nThe documentation further clarifies the callback behavior as follows:\r\n\r\n\u003e The `writable.write()` method writes some data to the stream, and calls the\r\n\u003e supplied `callback` once the data has been fully handled. If an error occurs,\r\n\u003e the callback _may or may not_ be called with the error as its first argument.\r\n\u003e To reliably detect write errors, add a listener for the `'error'` event. If\r\n\u003e callback is called with an error, it will be called before the `'error'` event\r\n\u003e is emitted.\r\n\r\nThis is quite accurate and inline with the actual implementation. There can only be a single write at time so if `writable.write()` is called while a write is in progress that write is buffered. If the stream errors or is destroyed the callbacks of buffered writes are not called.\r\n\r\nIn https://github.com/nodejs/node/pull/29028 and then in https://github.com/nodejs/node/pull/30596 this behavior was changed to call the callbacks of buffered writes with an error.\r\n\r\nIn my opinion this change was not need or better not justified by a specific issue. From what I've gathered (correct me if I am wrong) the main reason behind the change was that code like this\r\n\r\n```js\r\nawait for (const chunk of src) {\r\n  new Promise(function(resolve, reject) {\r\n    dst.write(chunk, err =\u003e err ? reject(err) : resolve());\r\n  });\r\n}\r\n```\r\n\r\ncould result in a promise that is never settled, but the destination stream in the example does not buffer writes so the callback is always called.\r\n\r\nI think this change creates more problems than it solves:\r\n\r\n- Not all callbacks must be called. A lot of callbacks are only called in response of a specific event / operation. If that event or operation never occurred its associated callback should not be called. This depends on case-by-case basis. In this specific case if the callback is called it makes me think that buffered writes are attempted even after a stream errors or is destroyed and this is not true even after https://github.com/nodejs/node/pull/30596.\r\n- AFAIK no issues have been created? about buffered write callbacks not being called on error in all these years so I think the original behavior is not broken.\r\n- There are some side effects like https://github.com/nodejs/node/issues/30841 or https://github.com/nodejs/node/pull/31179#issuecomment-570890897.\r\n- It is hard to adapt user land code to this new behavior. For example `net.Socket` buffered write callbacks are now called with an error if a write error occurs. Doing the same on Node.js \u003c 13 will be hard and hacky for userland libs. They should wait until Node.js 12 goes EOL to have the same behavior across all supported Node.js versions.\r\n\r\nRefs: https://github.com/nodejs/node/pull/29028\r\nRefs: https://github.com/nodejs/node/pull/30596\r\nRefs: https://github.com/nodejs/node/pull/31179\r\n","author":{"url":"https://github.com/lpinca","@type":"Person","name":"lpinca"},"datePublished":"2020-01-11T15:58:40.000Z","interactionStatistic":{"@type":"InteractionCounter","interactionType":"https://schema.org/CommentAction","userInteractionCount":14},"url":"https://github.com/31317/node/issues/31317"}