-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
[v13.x backport] stream: invoke buffered write callbacks on error #31179
Conversation
Buffered write callbacks were only invoked upon error if `autoDestroy` was invoked. Backport-PR-URL: nodejs#31179 PR-URL: nodejs#30596 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
3f9c037
to
03b85ec
Compare
@ronag PTAL. Two of the new tests are failing. |
The original PR had 3 TSC approvals so I think it's useless to ping TSC again but is it possible to get more opinions on this behavior before landing? My observations: |
Another problem after #31082 is that this calls const { createDeflateRaw } = require('zlib');
const data = Buffer.alloc(1024, 'a');
const deflate = createDeflateRaw();
deflate.resume();
deflate.write(data);
deflate.flush(function() {
throw new Error('Unexpected callback invocation');
});
process.nextTick(function() {
deflate.close();
}); Without this change |
@lpinca: What is your suggestion? Should we revert the original change in 13.x? If #30596 is not reverted then I believe this backport should land. Though, I think the current consensus going forward is that all callbacks should be invoked? Was this discussed by TSC btw? In my opinion the flush callback should be invoked with an error. Though I'm unsure whether that is currently the case and whether that is a breaking change. |
I don't know. Yes in my opinion calling buffered write callbacks with an error when a stream is destroyed/errors is a step in the wrong direction.
|
My main objection is that in an async API I would personally always expect the callback to be invoked. Otherwise patterns such as: await new Promise(w.write(chunk, err => err ? reject(err) : resolve());
await new Promise(delate.flush(err => err ? reject(err) : resolve()); Which I would expect to work and would seemingly "randomly" deadlock. Although I would find it very strange, I don't have a strong objection on this other than it should probably be more explicit in the docs if the callback might not be invoked.
Do we have any idea of how often this is the case? Whether these are a bugs or not I guess depends on how we want/expect it to work.
I feel like that is a different problem where we are not properly handling the "pending" state of a stream.
Good point. Though no issues does no necessarily mean not broken or no problems.
Good point. Not sure how big of a problem this is or not. This does kind of relate to an edge case so the ecosystem might or might not expect the callback to be invoked. The only time it is (or not) invoked is during a bit of an edge case. I don't know. Sorry for the hypotheticals. I guess we just need to gain some consensus on what the expectations should be. Maybe we should have a separate issue just focused on this and ping node/streams? |
No worries, this is a constructive discussion. I think it is ok for a promise like new Promise(function(resolve, reject) {
w.write(chunk, err => err ? reject(err) : resolve()
}); to never be settled because it means the stream already errored or was destroyed so no more writes are done.
It is a problem for me (guess what? |
Oh yea, sorry about that :D. I wouldn't mind helping out sorting these kind of breakage in
Let me extend the sample: EDIT: ugly/incomplete fix for const src = fs.createReadStream(...);
const dst = fs.createWriteStream(...);
try {
// Don't cause unhandled exception, we handle errors through the callback.
// This assumes that the _write implementation uses the callback instead of
// (incorrectly) calling .destroy(err) explicitly. Also assumes that no
// error occurs while opening the stream :/.
// Another alternative is to create an errorPromise and use Promise.race.
// But at that point there is no longer any need for cb to be invoked on error.
// Thus this example is slightly contrived.
dst.on('error', nop);
await for (const chunk of src) {
new Promise(function(resolve, reject) {
dst.write(chunk, err => err ? reject(err) : resolve());
});
}
await new Promise(function(resolve, reject) {
dst.end(err => err ? reject(err) : resolve());
});
} finally {
src.destroy(); // Uhoh? What if write/end never completes?
dst.destroy();
}
|
I think that code is broken in multiple ways. Not only because If users really want to shoot themselves in the foot with code like that they can destroy the source stream when the destination stream closes/errors. Also I think this is the reason why we have extensive documentation and utilities to pipe from async iterators. I don't think this is something that should be fixed because in my opinion it is not broken and works as intended. |
Async iterators do. But the writable side is indeed a bit problematic, which is why I've previously proposed that EDIT: Even with that errors during the "pending" state would not be handled through callbacks currently.
This is wrong. It does. Don't remember what version this was fixed, maybe 13.x?
I don't follow.
Though I feel were getting slightly off topic. You've found enough questions in my example to make me reconsider its relevance. |
In the example above the destination stream does not buffer anything so I'm not sure if it can actually hang. Only callbacks of buffered writes are not called if the stream is closed prematurely.
Yes, my bad sorry, too tired.
|
It might buffer if it is still waiting for the stream to be In terms of generally all callbacks should be invoked I don't know if that's really relevant.
Oh yes, that's actually pretty cool. My mistake. |
Though I would prefer if callback were always invoked. I'm alright (-0) with changing this back to how it was. Maybe with some more explicit docs. Makes things simpler implementation/maintenance wise which I appreciate. But I would like to have some more input from the rest of the @nodejs/streams team. |
@nodejs/streams |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
For completeness, the |
Ok, documentation must be updated then. |
Indeed. Created an issue (#31220). I will fix that once we agreed on what to do. If we go the revert direction, we might need to revert #29747 as well to be consistent. @lpinca: I'll try to ping you as well for these kinds of PR's in the future. Btw, did you get added to nodejs/streams? Also, I still think this discussion would be better in a dedicated issue. One way or another I believe the TSC might need to get involved once we've agreed on the options. |
Even though this is not correct. The example is still not very good since as you pointed out before it doesn't properly handle |
Yes.
Perhaps but afaik TSC prefers issues to be resolved by dedicated teams. |
Btw I think @mcollina is the only active member of the streams team. We really need to expand it. |
As a note: I won't include this PR in 13.6.0. @mcollina please have a closer look at the comments and maybe leave a comment about your opinion on the issue. |
@lpinca can you open up a separate issue on why we should rever that? I'm ok to consider it, but I generally feel that having a callback that is not reliable to be called is extremely problematic when writing values. |
Will do.
It is reliable until the stream errors, or is destroyed. After that buffered writes will stay buffered indefinitely. They will never be processed, so calling the callbacks of those writes is a mistake in my opinion. It's like calling the |
The moment we allow callbacks to not be called (if not stated/documented), then we are at risk of introducing memory leaks and hard to track bugs. We should probably call the callback with an error if the stream is already destroyed then. |
And we do if |
A write can be buffered before the writable is |
Wait for |
1c8bccf
to
6055134
Compare
Landed in d0a0071. |
Refs: #30596 Buffered write callbacks were only invoked upon error if `autoDestroy` was invoked. Backport-PR-URL: #31179 PR-URL: #30596 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
Buffered write callbacks were only invoked upon error if
autoDestroy
was invoked.Refs: #30596
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes