http2: don't throw when destroying socket proxy

Calling destroy() on the http2session.socket proxy now destroys the
session instead of throwing ERR_HTTP2_NO_SOCKET_MANIPULATION.

Refs: nodejs/undici#5525
Signed-off-by: Matteo Collina <hello@matteocollina.com>

Author: mcollina

doc/api/http2.md

@@ -722,18 +722,24 @@ registered as a listener on the `'timeout'` event.
 
 <!-- YAML
 added: v8.4.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/64427
+    description: Calling `destroy` no longer throws and instead destroys the
+                 `Http2Session`.
 -->
 
 * Type: {net.Socket|tls.TLSSocket}
 
 Returns a `Proxy` object that acts as a `net.Socket` (or `tls.TLSSocket`) but
 limits available methods to ones safe to use with HTTP/2.
 
-`destroy`, `emit`, `end`, `pause`, `read`, `resume`, and `write` will throw
+`emit`, `end`, `pause`, `read`, `resume`, and `write` will throw
 an error with code `ERR_HTTP2_NO_SOCKET_MANIPULATION`. See
 [`Http2Session` and Sockets][] for more information.
 
-`setTimeout` method will be called on this `Http2Session`.
+`destroy`, `setTimeout`, `ref`, and `unref` methods will be called on this
+`Http2Session`.
 
 All other interactions will be routed directly to the socket.
 

lib/internal/http2/core.js

@@ -984,8 +984,8 @@ const proxySocketHandler = {
       case 'setTimeout':
       case 'ref':
       case 'unref':
-        return session[prop].bind(session);
       case 'destroy':
+        return session[prop].bind(session);
       case 'emit':
       case 'end':
       case 'pause':
@@ -1018,9 +1018,9 @@ const proxySocketHandler = {
       case 'setTimeout':
       case 'ref':
       case 'unref':
+      case 'destroy':
         session[prop] = value;
         return true;
-      case 'destroy':
       case 'emit':
       case 'end':
       case 'pause':

test/parallel/test-http2-socket-proxy-destroy.js

@@ -0,0 +1,57 @@
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+const assert = require('assert');
+const h2 = require('http2');
+
+// Calling .destroy() on the socket proxy must not throw
+// (previously threw ERR_HTTP2_NO_SOCKET_MANIPULATION) and must
+// destroy the Http2Session and the underlying socket.
+
+const server = h2.createServer();
+
+server.on('stream', common.mustCall(function(stream) {
+  stream.respond();
+  stream.end('ok');
+}, 2));
+
+server.listen(0, common.mustCall(() => {
+  const port = server.address().port;
+
+  // Test destroy() without an error.
+  {
+    const client = h2.connect(`http://localhost:${port}`);
+    const request = client.request();
+    request.resume();
+    request.on('close', common.mustCall(() => {
+      const socket = client.socket;
+      socket.destroy();
+      assert.strictEqual(client.destroyed, true);
+      client.on('close', common.mustCall(() => {
+        assert.strictEqual(client.socket, undefined);
+        // Destroying again through a stale proxy reference must not throw.
+        socket.destroy();
+      }));
+    }));
+    client.on('error', common.mustNotCall());
+  }
+
+  // Test destroy() with an error.
+  {
+    const client = h2.connect(`http://localhost:${port}`);
+    const request = client.request();
+    request.resume();
+    request.on('close', common.mustCall(() => {
+      client.socket.destroy(new Error('boom'));
+      assert.strictEqual(client.destroyed, true);
+    }));
+    client.on('error', common.mustCall((err) => {
+      assert.strictEqual(err.message, 'boom');
+    }));
+    client.on('close', common.mustCall(() => {
+      server.close();
+    }));
+  }
+}));

test/parallel/test-http2-socket-proxy.js

@@ -52,7 +52,8 @@ server.on('stream', common.mustCall(function(stream, headers) {
   assert(inspectedTimersList.includes('      _idleNext: [Timeout]'));
   assert(!inspectedTimersList.includes('       _idleNext: [Timeout]'));
 
-  assert.throws(() => socket.destroy, errMsg);
+  assert.strictEqual(typeof socket.destroy, 'function');
+
   assert.throws(() => socket.emit, errMsg);
   assert.throws(() => socket.end, errMsg);
   assert.throws(() => socket.pause, errMsg);
@@ -63,7 +64,6 @@ server.on('stream', common.mustCall(function(stream, headers) {
   assert.throws(() => socket.setKeepAlive, errMsg);
   assert.throws(() => socket.setNoDelay, errMsg);
 
-  assert.throws(() => (socket.destroy = undefined), errMsg);
   assert.throws(() => (socket.emit = undefined), errMsg);
   assert.throws(() => (socket.end = undefined), errMsg);
   assert.throws(() => (socket.pause = undefined), errMsg);