http2: throw better error when accessing unbound socket proxy by jasnell · Pull Request #22486 · nodejs/node · GitHub

Fixes the bug in the sense that a better error message is provided. Generally speaking, once the connection has been closed, users should not continue to the use the socket proxy object. So, throw whenever they try to.

Fixes: #22268

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

@jasnell build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/651/pipeline

There's another ways to expose the original socket:

session.socket.ref()
session.socket.unref()
session.socket.setEncoding()
session.socket.setKeepAlive()
session.socket.setNoDelay()

Should these be fixed?

I am not sure if this is the ideal solution but the code looks good to me.

What is expected to happen sync?

What do you mean?

Should these be fixed?

Yeah, I'll take another look.

Fixes: nodejs#22268

Added additional tests for the ref(), unref(), setEncoding(), setKeepAlive(), and setNoDelay() functions.

CI: https://ci.nodejs.org/job/node-test-pull-request/16733/

Resume Build: https://ci.nodejs.org/job/node-test-pull-request/16749/

Theoretically semver-major (I think?) if it doesn't end up in the same release as the "HTTP/2 is stable!" thing, so let's get this thing landed.

I personally don't know that I would have chosen an HTTP2 error for this (and others) instead of one of the existing SOCKET errors, but maybe more specific is good and I make bad decisions. 🙃

@nodejs/http2

LGTM

Fixes: #22268 PR-URL: #22486 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Landed in dd03706

Fixes: #22268 PR-URL: #22486 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

@jasnell I think you missed something :P

You can't do session.socket.destroy() because it'll throw ERR_HTTP2_NO_SOCKET_MANIPULATION, but you can do session.socket.ref().destroy(). As I said, these functions expose the original socket:

session.socket.ref()
session.socket.unref()
session.socket.setEncoding()
session.socket.setKeepAlive()
session.socket.setNoDelay()

So ProxySocket should look like:

  get(session, prop) {
    switch (prop) {
      case 'setTimeout':
        return session.setTimeout.bind(session);
      case 'destroy':
      case 'emit':
      case 'end':
      case 'pause':
      case 'read':
      case 'resume':
      case 'write':
      case 'setEncoding':
      case 'setKeepAlive':
      case 'setNoDelay':
        throw new ERR_HTTP2_NO_SOCKET_MANIPULATION();
      case 'ref':
      case 'unref':
        return socket[prop].bind(session.socket);
      default:
        const socket = session[kSocket];
        if (socket === undefined)
          throw new ERR_HTTP2_SOCKET_UNBOUND();
        const value = socket[prop];
        return typeof value === 'function' ? value.bind(socket) : value;
    }
  }
...
  set(session, prop, value) {
    switch (prop) {
      case 'setTimeout':
        session.setTimeout = value;
        return true;
      case 'destroy':
      case 'emit':
      case 'end':
      case 'pause':
      case 'read':
      case 'resume':
      case 'write':
      case 'setEncoding':
      case 'setKeepAlive':
      case 'setNoDelay':
      case 'ref':
      case 'unref':
        throw new ERR_HTTP2_NO_SOCKET_MANIPULATION();
      default:
        const socket = session[kSocket];
        if (socket === undefined)
          throw new ERR_HTTP2_SOCKET_UNBOUND();
        socket[prop] = value;
        return true;
    }

@szmarczak good spot! would you like to open a PR to fix that?

Yeah, sure.

Refs: nodejs#22486

@mcollina Here it is: #22650

Fixes: #22268 PR-URL: #22486 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Refs: #22486 PR-URL: #22650 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Fixes: #22268 PR-URL: #22486 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Refs: #22486 PR-URL: #22650 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Fixes: nodejs#22268 PR-URL: nodejs#22486 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Refs: nodejs#22486 PR-URL: nodejs#22650 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Fixes: nodejs#22268 PR-URL: nodejs#22486 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Refs: nodejs#22486 PR-URL: nodejs#22650 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Fixes: #22268 Backport-PR-URL: #22850 PR-URL: #22486 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Refs: #22486 Backport-PR-URL: #22850 PR-URL: #22650 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added dont-land-on-v6.x errors Issues and PRs related to JavaScript errors originated in Node.js core. http2 Issues or PRs related to the http2 subsystem. labels Aug 23, 2018

jasnell mentioned this pull request Aug 23, 2018

Accessing socket after HTTP2 session gets destroyed throws an error #22268

Closed

BridgeAR approved these changes Aug 23, 2018

View reviewed changes

szmarczak mentioned this pull request Aug 24, 2018

http2: graduate from experimental #22466

Closed

3 tasks

http2: throw better error when accessing unbound socket proxy

dc92650

Fixes: nodejs#22268

jasnell force-pushed the http2-proxy-socket-unbound branch from 3bb3522 to dc92650 Compare August 24, 2018 18:27

jasnell added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Aug 24, 2018

Trott approved these changes Aug 25, 2018

View reviewed changes

trivikr approved these changes Aug 25, 2018

View reviewed changes

mcollina approved these changes Aug 25, 2018

View reviewed changes

jasnell closed this Aug 27, 2018

szmarczak added a commit to szmarczak/node that referenced this pull request Sep 2, 2018

http2: don't expose the original socket through the socket proxy

b4bebab

Refs: nodejs#22486

szmarczak mentioned this pull request Sep 2, 2018

http2: don't expose the original socket through the socket proxy #22650

Closed

3 tasks

targos mentioned this pull request Sep 5, 2018

Release proposal: v10.10.0 #22716

Merged

This was referenced Sep 6, 2018

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-android-nodejs#236

Closed

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-nodejs#221

Closed

BethGriggs mentioned this pull request Oct 30, 2018

v8.13.0 proposal #23974

Merged

beevelop mentioned this pull request Nov 20, 2018

2018-11-20 Version 8.13.0 'Carbon' (LTS) @MylesBorins prepared by @BethGriggs beevelop/docker-android-nodejs#250

Closed

http2: throw better error when accessing unbound socket proxy by jasnell · Pull Request #22486 · nodejs/node · GitHub

Uh oh!

Uh oh!

http2: throw better error when accessing unbound socket proxy - #22486

http2: throw better error when accessing unbound socket proxy#22486jasnell wants to merge 1 commit intonodejs:masternodejs/node:masterfrom jasnell:http2-proxy-socket-unboundjasnell/node:http2-proxy-socket-unboundCopy head branch name to clipboard

Conversation

jasnell commented Aug 23, 2018

Checklist

Uh oh!

nodejs-github-bot commented Aug 23, 2018

Uh oh!

szmarczak commented Aug 23, 2018

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR Aug 23, 2018

Choose a reason for hiding this comment

Uh oh!

jasnell Aug 23, 2018

Choose a reason for hiding this comment

Uh oh!

jasnell commented Aug 23, 2018

Uh oh!

jasnell commented Aug 24, 2018

Uh oh!

jasnell commented Aug 24, 2018

Uh oh!

Trott commented Aug 25, 2018

Uh oh!

Trott commented Aug 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Aug 25, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Aug 27, 2018

Uh oh!

szmarczak commented Aug 30, 2018

Uh oh!

mcollina commented Aug 30, 2018

Uh oh!

szmarczak commented Aug 30, 2018

Uh oh!

szmarczak commented Sep 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

http2: throw better error when accessing unbound socket proxy#22486
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:http2-proxy-socket-unbound

Trott commented Aug 25, 2018 •
edited
Loading