test: fix racey-ness in tls-inception #1040
Conversation
Fix test failure on FreeBSD and SmartOS, which happens due to a bad
timing:
events.js:141
throw er; // Unhandled 'error' event
^
Error: read ECONNRESET
at exports._errnoException (util.js:734:11)
at TLSWrap.onread (net.js:538:26)
The outer `net.conncet()` socket stays alive after the inner socket is
gone. This happens because `.pipe()`'s implementation does not `destroy`
the source side when the destination has emitted `close`.
Fix: nodejs#1012
|
cc @jbergstroem |
|
Confirmed that the patch indeed fixes the race on FreeBSD and SmartOS (solaris). Thanks, @indutny. |
|
Good! Please review, @iojs/crypto @iojs/collaborators |
|
If calling socket.destroy() explicitly on close fixes it, doesn't that suggest there's a streams bug somewhere? /cc @chrisdickinson. |
|
@bnoordhuis it might be that the socket was closed without sending the shutdown on it. At least it doesn't get EOF event, so here we have ECONNRESET. |
|
"close" and "destroy" are pretty underspecified per base streams. "close" will unpipe any upstream stream from the emitter. If a streams1 stream is the emitter, it will attempt to call |
|
|
||
| dest.on('close', function() { | ||
| socket.destroy(); | ||
| }); |
There was a problem hiding this comment.
Ah! that's the problem. dest is a streams3 stream, which does not call destroy on downstream streams on close. streams1 streams would call destroy on downstream streams.
|
I guess my only remaining question is: should we set up |
|
@chrisdickinson it should not happen under test conditions. |
|
LGTM, then. |
|
@bnoordhuis LGTY too? |
|
Um, I suppose so. @chrisdickinson's explanation about streams1 vs. streams3 objects makes it sound like there is a loaded gun waiting to blow off the feet of anyone mixing them in an application. I find it slightly worrying. |
Fix test failure on FreeBSD and SmartOS, which happens due to a bad
timing:
events.js:141
throw er; // Unhandled 'error' event
^
Error: read ECONNRESET
at exports._errnoException (util.js:734:11)
at TLSWrap.onread (net.js:538:26)
The outer `net.conncet()` socket stays alive after the inner socket is
gone. This happens because `.pipe()`'s implementation does not `destroy`
the source side when the destination has emitted `close`.
Fix: #1012
PR-URL: #1040
Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>
|
Landed in e1bf670, thank you! |
Fix test failure on FreeBSD and SmartOS, which happens due to a bad
timing:
The outer
net.conncet()socket stays alive after the inner socket isgone. This happens because
.pipe()'s implementation does notdestroythe source side when the destination has emitted
close.Fix: #1012