Always call callbacks in outgoing streams #2240
Conversation
laino
changed the title
|
Why isn't this handled by the base stream implementation? |
|
@mikeal As far as I understand http outgoing streams have their own internal buffer. Data/Callbacks in that buffer haven't been written to the underlying "base" stream yet, so they won't be called with an error should the underlying stream close prematurely (i.e before the data was written). This PR fixes that. |
|
Yup, let me clarify a bit. I'm not commenting about the PR in a +1/-1 context, this is a bug and should be fixed and someone with more familiarity with the "gotchas" of new style streams should review it (in other words: not me). I'm posing more of a question and possibly some longer term work (outside this PR) that should not interfere with this PR getting merged. I'm wondering why we can't inherit this functionality from the base stream implementation? One of the major reasons for the streams rewrite was to provide a base we could actually use across all of stdlib. At the time of the rewrite many of the streams in core didn't inherit from core streams either for legacy reasons or because they needed to do something "special." Streams 2/3 dramatically complicated the internal stream API and issues like this will run rampant if everyone can't use the base classes effectively. To be reminded that we still have internal streams that are implementing one-off behavior not covered by the base streams and that we are still finding issues like this is troubling. Perhaps we should add an Issue in the /cc @nodejs/streams |
|
|
||
| var callbackCalled = false; | ||
|
|
||
| res.end('ok', function() { |
There was a problem hiding this comment.
You can simply use res.end('ok', common.mustCall(function() {})). It will fail if the function is not called exactly once.
|
Quick review looks clean. Let's run CI and see how it does. Also, might need to be rebased. |
laino
force-pushed
the
fix-stream-callbacks
branch
from
e39946d
to
f8a4e16
Compare
|
Rebased the PR and added changes proposed by @thefourtheye |
|
CI: https://ci.nodejs.org/job/node-test-pull-request/236/ @laino The commit message will have to be updated to include subsystem, title and then a message body describing what's being fixed. |
|
CI hit a problem with a RPi, it's waiting for solution but marked offline for now. Started again: https://ci.nodejs.org/job/node-test-pull-request/238/ |
| var err = new Error('write after end'); | ||
| process.nextTick(function() { | ||
| self.emit('error', err); | ||
| if (callback) callback(err); |
There was a problem hiding this comment.
I feel like this is a strange hybrid between err-back and event emitter.
/cc @nodejs/tsc Feedback on basically emitting the error twice?
There was a problem hiding this comment.
One or the other seems better. Maybe if (typeof callback === 'function') callback(err); else self.emit('error', err);
There was a problem hiding this comment.
I think this had been discussed some time back. See nodejs/node-v0.x-archive#7477 where this change originated from.
IMHO it should only be one or the other, like @cjihrig says. If a callback is supplied, then pass the error and not emit. Otherwise emit.
There was a problem hiding this comment.
I like that solution. @laino mind making that change?
|
Two tests failed in CI:
|
|
@joaocgreis The CI system has been giving me sporadic errors all over the place today. I doubt it's related. |
|
One more test run: https://ci.nodejs.org/job/node-test-pull-request/239/ ... to double check test-net-listen-shared-ports |
|
Awesome. All green. @laino If you can make the change in emitting the error I think this will be ready to land. |
| // We *might* get a socket already closed error here, which | ||
| // occurs when the socket was destroyed before we finished | ||
| // writing our data. | ||
| res.on('error', function() {}); |
There was a problem hiding this comment.
Then lets check the error here, just to be sure.
There was a problem hiding this comment.
But should that error cause the test to fail? I don't believe this particular fail means what the test was created to test has failed.
There was a problem hiding this comment.
As it is we simply ignore everything right? As the comment says, if we check if the error is actually socket closed error, wouldn't it be better?
There was a problem hiding this comment.
oh I see what you're saying. good point. yes, checking for expected errors, but still throwing on unexpected errors, would be a smart thing to do.
|
@laino ... ping... there were a few additional comments here that needed to be addressed; and this PR would need to be rebased and updated before it could land. Thank you! |
|
Ping @laino |
estliberitas
force-pushed
the
master
branch
2 times, most recently
from
7da4fd4
to
c7066fb
Compare
|
Closing given the lack of any further activity. The original issue is still open. Can reopen this if someone wishes to take it over but it would really need a new PR at this point. |
Fixes: #1047
This is basically the same PR as #1143, but it includes some proposed changes.