-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
fix(ext/node/net): emit error
before close
when connection is refused
#24656
Conversation
error
before close
when destroying socketerror
before close
when connection is refused
@bartlomieju PTAL |
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, thanks for looking into this one!
@@ -40,7 +38,7 @@ export class HandleWrap extends AsyncWrap { | |||
|
|||
close(cb: () => void = () => {}) { | |||
this._onClose(); | |||
queueMicrotask(cb); | |||
setTimeout(cb, 0); |
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.
Have you tried using nextTick
instead?
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.
nextTick
doesn't delay enough because emitErrorNT
(which causes error
event) callback is called at next tick (and that's registered after _handle.close
callback.
deno/ext/node/polyfills/net.ts
Lines 1418 to 1426 in 29934d5
this._handle.close(() => { | |
this._handle!.onread = _noop; | |
this._handle = null; | |
this._sockname = undefined; | |
debug("emit close"); | |
this.emit("close", isException); | |
}); | |
cb(exception); |
In the above code, the callback for this._handle.close
is the one delayed in this PR (that emits close
). cb
in the above is onDestroy
in _stream.mjs
, which calls process.nextTick(() => stream.emit("error"))
.
So we need to delay the close callback at least 2 ticks.
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.
I updated the line to nextTick(nextTick, cb)
which also seems working, and also feels more accurate/predictable than setTimeout
.
This PR delays the timing of callback called in
socket._handle.close(cb)
call, and causeserror
event happens beforeclose
event whensocket.destroy()
is called.npm:mongodb
package (A node.js stream polyfill issue: the error thrown within the process.nextTick callback cannot be caught, leading to unexpected exit when connection closed #24376, error: Uncaught Error: read ECONNRESET #19078).closes #24376
closes #23665
closes #21951
closes #19078
Note: The changes in
unit_node/tls_test.ts
,unit_node/http_test.ts
, andpolyfills/dgram.ts
were needed for avoiding op leak error in testing. They are not directly related to the fix.TODO:
enable test casesWrite test cases (I couldn't find the exact test case which exercise this behavior ofnet.createConnection
in node.js native test cases)