From 652514233fe8a30adab7bd0b0e340f49a178eae9 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Thu, 31 Oct 2019 15:48:16 +0100
Subject: [PATCH] http,async_hooks: keep resource object alive from socket

If `asyncReset()` is used to specify an alternative resource object
to mark a re-used socket in the HTTP Agent implementation,
store that object and keep it alive, because domains rely on GC tracking
for resource objects to manage their own lifetimes, and previously that
resource object might have been garbage-collected too early, leading to
crashes.

Fixes: https://github.com/nodejs/node/issues/30122

PR-URL: https://github.com/nodejs/node/pull/30196
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
---
 lib/_http_agent.js                            |  8 ++
 .../test-http-agent-domain-reused-gc.js       | 98 +++++++++++++++++++
 2 files changed, 106 insertions(+)
 create mode 100644 test/parallel/test-http-agent-domain-reused-gc.js

diff --git a/lib/_http_agent.js b/lib/_http_agent.js
index e1cfa6d7fc9d17..48bdf98088931f 100644
--- a/lib/_http_agent.js
+++ b/lib/_http_agent.js
@@ -40,10 +40,18 @@ const { async_id_symbol } = require('internal/async_hooks').symbols;
 // ClientRequest.onSocket(). The Agent is now *strictly*
 // concerned with managing a connection pool.
 
+const kReusedHandle = Symbol('kReusedHandle');
 class ReusedHandle {
   constructor(type, handle) {
     this.type = type;
     this.handle = handle;
+    // We need keep the resource object alive from this object, because
+    // domains rely on GC of the resource object for lifetime tracking.
+    // TODO(addaleax): This should really apply to all uses of
+    // AsyncWrap::AsyncReset() when the resource is not the AsyncWrap object
+    // itself. However, HTTPClientAsyncResource and HTTPServerAsyncResource
+    // hold on to other objects, inhibiting GC.
+    handle[kReusedHandle] = this;
   }
 }
 
diff --git a/test/parallel/test-http-agent-domain-reused-gc.js b/test/parallel/test-http-agent-domain-reused-gc.js
new file mode 100644
index 00000000000000..9470b23b472662
--- /dev/null
+++ b/test/parallel/test-http-agent-domain-reused-gc.js
@@ -0,0 +1,98 @@
+// Flags: --expose-gc --expose-internals
+'use strict';
+const common = require('../common');
+const http = require('http');
+const async_hooks = require('async_hooks');
+const makeDuplexPair = require('../common/duplexpair');
+
+// Regression test for https://github.com/nodejs/node/issues/30122
+// When a domain is attached to an http Agent’s ReusedHandle object, that
+// domain should be kept alive through the ReusedHandle and that in turn
+// through the actual underlying handle.
+
+// Consistency check: There is a ReusedHandle being used, and it emits events.
+// We also use this async hook to manually trigger GC just before the domain’s
+// own `before` hook runs, in order to reproduce the bug above (the ReusedHandle
+// being collected and the domain with it while the handle is still alive).
+const checkInitCalled = common.mustCall();
+const checkBeforeCalled = common.mustCallAtLeast();
+let reusedHandleId;
+async_hooks.createHook({
+  init(id, type, triggerId, resource) {
+    if (resource.constructor.name === 'ReusedHandle') {
+      reusedHandleId = id;
+      checkInitCalled();
+    }
+  },
+  before(id) {
+    if (id === reusedHandleId) {
+      global.gc();
+      checkBeforeCalled();
+    }
+  }
+}).enable();
+
+// We use a DuplexPair rather than TLS sockets to keep the domain from being
+// attached to too many objects that use strong references (timers, the network
+// socket handle, etc.) and wrap the client side in a JSStreamSocket so we don’t
+// have to implement the whole _handle API ourselves.
+const { serverSide, clientSide } = makeDuplexPair();
+const JSStreamSocket = require('internal/js_stream_socket');
+const wrappedClientSide = new JSStreamSocket(clientSide);
+
+// Consistency check: We use asyncReset exactly once.
+wrappedClientSide._handle.asyncReset =
+  common.mustCall(wrappedClientSide._handle.asyncReset);
+
+// Dummy server implementation, could be any server for this test...
+const server = http.createServer(common.mustCall((req, res) => {
+  res.writeHead(200, {
+    'Content-Type': 'text/plain'
+  });
+  res.end('Hello, world!');
+}, 2));
+server.emit('connection', serverSide);
+
+// HTTP Agent that only returns the fake connection.
+class TestAgent extends http.Agent {
+  createConnection = common.mustCall(() => wrappedClientSide)
+}
+const agent = new TestAgent({ keepAlive: true, maxSockets: 1 });
+
+function makeRequest(cb) {
+  const req = http.request({ agent }, common.mustCall((res) => {
+    res.resume();
+    res.on('end', cb);
+  }));
+  req.end('');
+}
+
+// The actual test starts here:
+
+const domain = require('domain');
+// Create the domain in question and a dummy “noDomain” domain that we use to
+// avoid attaching new async resources to the original domain.
+const d = domain.create();
+const noDomain = domain.create();
+
+d.run(common.mustCall(() => {
+  // Create a first request only so that we can get a “re-used” socket later.
+  makeRequest(common.mustCall(() => {
+    // Schedule the second request.
+    setImmediate(common.mustCall(() => {
+      makeRequest(common.mustCall(() => {
+        // The `setImmediate()` is run inside of `noDomain` so that it doesn’t
+        // keep the actual target domain alive unnecessarily.
+        noDomain.run(common.mustCall(() => {
+          setImmediate(common.mustCall(() => {
+            // This emits an async event on the reused socket, so it should
+            // run the domain’s `before` hooks.
+            // This should *not* throw an error because the domain was garbage
+            // collected too early.
+            serverSide.end();
+          }));
+        }));
+      }));
+    }));
+  }));
+}));