fix(ext/node/net): emit `error` before `close` when connection is refused (#24656)

5 files changed, 48 insertions, 16 deletions

diff --git a/ext/node/polyfills/dgram.ts b/ext/node/polyfills/dgram.ts
index 17ec4f2c3..99f4940ec 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 fd17a1edb..ef8457338 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 b9fe767e6..3831cac06 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<void>();
+    const { promise: serverClosed, resolve: resolveServer } = Promise
+      .withResolvers<void>();
+    const { promise: socketClosed, resolve: resolveSocket } = Promise
+      .withResolvers<void>();
     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 89a9fb6ba..ebbc749b5 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<void>();
@@ -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<void>();
+  const closeEmitted = Promise.withResolvers<void>();
+  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<void>();
 
diff --git a/tests/unit_node/tls_test.ts b/tests/unit_node/tls_test.ts
index db954f328..a9dc10c20 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<void>();
   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;
 });