From b57789687bf1ce555d25894540456163a9a376c8 Mon Sep 17 00:00:00 2001 From: Yoshiya Hinosawa Date: Wed, 24 Jul 2024 20:33:45 +0900 Subject: [PATCH] fix(ext/node/net): emit `error` before `close` when connection is refused (#24656) --- ext/node/polyfills/dgram.ts | 17 ++++++++------- .../polyfills/internal_binding/handle_wrap.ts | 7 ++++--- tests/unit_node/http_test.ts | 13 +++++++++--- tests/unit_node/net_test.ts | 21 ++++++++++++++++++- tests/unit_node/tls_test.ts | 6 +++++- 5 files changed, 48 insertions(+), 16 deletions(-) diff --git a/ext/node/polyfills/dgram.ts b/ext/node/polyfills/dgram.ts index 17ec4f2c38..99f4940ec2 100644 --- a/ext/node/polyfills/dgram.ts +++ b/ext/node/polyfills/dgram.ts @@ -531,16 +531,17 @@ export class Socket extends EventEmitter { healthCheck(this); stopReceiving(this); - state.handle!.close(); + state.handle!.close(() => { + // Deviates from the Node implementation to avoid leaking the timer ops at 'close' event + defaultTriggerAsyncIdScope( + this[asyncIdSymbol], + nextTick, + socketCloseNT, + this, + ); + }); state.handle = null; - defaultTriggerAsyncIdScope( - this[asyncIdSymbol], - nextTick, - socketCloseNT, - this, - ); - return this; } diff --git a/ext/node/polyfills/internal_binding/handle_wrap.ts b/ext/node/polyfills/internal_binding/handle_wrap.ts index fd17a1edb1..ef8457338f 100644 --- a/ext/node/polyfills/internal_binding/handle_wrap.ts +++ b/ext/node/polyfills/internal_binding/handle_wrap.ts @@ -25,13 +25,12 @@ // - https://github.com/nodejs/node/blob/master/src/handle_wrap.h // TODO(petamoriken): enable prefer-primordials for node polyfills -// deno-lint-ignore-file prefer-primordials - import { unreachable } from "ext:deno_node/_util/asserts.ts"; import { AsyncWrap, providerType, } from "ext:deno_node/internal_binding/async_wrap.ts"; +import { nextTick } from "ext:deno_node/_process/process.ts"; export class HandleWrap extends AsyncWrap { constructor(provider: providerType) { @@ -40,7 +39,9 @@ export class HandleWrap extends AsyncWrap { close(cb: () => void = () => {}) { this._onClose(); - queueMicrotask(cb); + // We need to delay 'cb' at least 2 ticks to avoid "close" event happenning before "error" event in net.Socket + // See https://github.com/denoland/deno/pull/24656 for more information + nextTick(nextTick, cb); } ref() { diff --git a/tests/unit_node/http_test.ts b/tests/unit_node/http_test.ts index b9fe767e6f..3831cac062 100644 --- a/tests/unit_node/http_test.ts +++ b/tests/unit_node/http_test.ts @@ -846,7 +846,10 @@ Deno.test( "[node/http] client upgrade", { permissions: { net: true } }, async () => { - const { promise, resolve } = Promise.withResolvers(); + const { promise: serverClosed, resolve: resolveServer } = Promise + .withResolvers(); + const { promise: socketClosed, resolve: resolveSocket } = Promise + .withResolvers(); const server = http.createServer((req, res) => { // @ts-ignore: It exists on TLSSocket assert(!req.socket.encrypted); @@ -887,12 +890,16 @@ Deno.test( // @ts-ignore it's a socket for real serverSocket!.end(); server.close(() => { - resolve(); + resolveServer(); + }); + socket.on("close", () => { + resolveSocket(); }); }); }); - await promise; + await serverClosed; + await socketClosed; }, ); diff --git a/tests/unit_node/net_test.ts b/tests/unit_node/net_test.ts index 89a9fb6bab..ebbc749b5b 100644 --- a/tests/unit_node/net_test.ts +++ b/tests/unit_node/net_test.ts @@ -5,7 +5,7 @@ import { assert, assertEquals } from "@std/assert/mod.ts"; import * as path from "@std/path/mod.ts"; import * as http from "node:http"; -Deno.test("[node/net] close event emits after error event", async () => { +Deno.test("[node/net] close event emits after error event - when host is not found", async () => { const socket = net.createConnection(27009, "doesnotexist"); const events: ("error" | "close")[] = []; const errorEmitted = Promise.withResolvers(); @@ -24,6 +24,25 @@ Deno.test("[node/net] close event emits after error event", async () => { assertEquals(events, ["error", "close"]); }); +Deno.test("[node/net] close event emits after error event - when connection is refused", async () => { + const socket = net.createConnection(27009, "127.0.0.1"); + const events: ("error" | "close")[] = []; + const errorEmitted = Promise.withResolvers(); + const closeEmitted = Promise.withResolvers(); + socket.once("error", () => { + events.push("error"); + errorEmitted.resolve(); + }); + socket.once("close", () => { + events.push("close"); + closeEmitted.resolve(); + }); + await Promise.all([errorEmitted.promise, closeEmitted.promise]); + + // `error` happens before `close` + assertEquals(events, ["error", "close"]); +}); + Deno.test("[node/net] the port is available immediately after close callback", async () => { const deferred = Promise.withResolvers(); diff --git a/tests/unit_node/tls_test.ts b/tests/unit_node/tls_test.ts index db954f3286..a9dc10c205 100644 --- a/tests/unit_node/tls_test.ts +++ b/tests/unit_node/tls_test.ts @@ -48,6 +48,7 @@ for ( conn.close(); outgoing.destroy(); listener.close(); + await new Promise((resolve) => outgoing.on("close", resolve)); }); } @@ -93,6 +94,7 @@ Connection: close // https://github.com/denoland/deno/pull/20120 Deno.test("tls.connect mid-read tcp->tls upgrade", async () => { + const { promise, resolve } = Promise.withResolvers(); const ctl = new AbortController(); const serve = Deno.serve({ port: 8443, @@ -119,8 +121,10 @@ Deno.test("tls.connect mid-read tcp->tls upgrade", async () => { conn.destroy(); ctl.abort(); }); + conn.on("close", resolve); await serve.finished; + await promise; }); Deno.test("tls.createServer creates a TLS server", async () => { @@ -136,6 +140,7 @@ Deno.test("tls.createServer creates a TLS server", async () => { socket.destroy(); } }); + socket.on("close", () => deferred.resolve()); }, ); server.listen(0, async () => { @@ -166,7 +171,6 @@ Deno.test("tls.createServer creates a TLS server", async () => { conn.close(); server.close(); - deferred.resolve(); }); await deferred.promise; });