http2: request.socket|connection

[sebdeckers]

As discussed in: #125 (comment)

Adds request.socket and request.connection for compatibility with documented HTTP APIs:

• message.socket
  Added in: v0.3.0
  <net.Socket>
  The net.Socket object associated with the connection.
  https://nodejs.org/api/http.html#http_message_socket

• Event: 'connection'
  [...]
  The socket can also be accessed at request.connection.
  https://nodejs.org/api/http.html#http_event_connection

Use case: I found that popular packages like cookies (NPM: >500k/mo) make use of this to detect the TLS context.

Checklist

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

Affected core subsystem(s)

http2

[sebdeckers]

@mcollina Do you mean just replicate that logic in our test, or to actually pull in the cookies package from NPM and use it in a test? (If the latter, what is the recommended way to add 3rd party packages to the node repo?)

[mcollina]

We need a line that just does this: https://github.com/pillarjs/cookies/blob/master/index.js#L87

[sebdeckers]

Okay, I'm just doing a look around for other examples.

finalhandler (NPM: 14M/mo)

req.socket.destroy()

https://github.com/pillarjs/finalhandler/blob/master/index.js#L130

express (NPM: ∞/mo)

req.connection.encrypted = true;

https://github.com/expressjs/express/blob/master/test/req.protocol.js#L41

var proto = this.connection.encrypted
  ? 'https'
  : 'http';
var trust = this.app.get('trust proxy fn');

if (!trust(this.connection.remoteAddress, 0)) {
  return proto;
}

https://github.com/expressjs/express/blob/master/lib/request.js#L307-L314

[mcollina]

https://github.com/pillarjs/finalhandler/blob/master/index.js#L127-L132 cannot be supported as HTTP1 specific. In that specific case for HTTP1, we would have to destroy the stream, not the connection.

finalhandler also uses res._header  which was not documented in core. The one we should support is https://nodejs.org/api/http.html#http_response_headerssent. I will send a PR to finalhandler.

[sebdeckers]

hapi

    // Setup timeout

    if (this.raw.req.socket &&
        this.route.settings.timeout.socket !== undefined) {

        this.raw.req.socket.setTimeout(this.route.settings.timeout.socket || 0);    // Value can be false or positive
    }

https://github.com/hapijs/hapi/blob/master/lib/request.js#L344-L350

[mcollina]

I'm convinced. We need this :).

In the test, can you check that whatever socket  returns is correct? Just asserting its truthness is not enough.

[sebdeckers]

Added a simple typeof check which should cover all the public interfaces in use today.

Should I add a test case for HTTPS too? There's not really a difference in core/compat (both are just Http2Session). And tls.TLSSocket is a subclass of net.Socket anyway.

[mcollina]

that's fine as it is

[jasnell]

Regarding the hapi setTimeout example... I'm curious why it is necessary actually. In the http/2 case, the Http2Session instance itself has a setTimeout() and there is a 1-to-1 relationship between the Socket and the Session. Setting the timeout on the Http2Session should have the same basic effect, then. In fact, it may be better to accept have setTimeout() on Http2Session defer to the setTimeout() on the socket rather than it being a separate implementation.

[sebdeckers]

@jasnell I suspect it stems from wanting to set different timeouts for specific Hapi routes. See: hapijs/hapi#2195 It would only be possible to apply the timeout once the request has been parsed, not at the initial socket connection listener. Just a guess.

[jasnell]

Yeah, I get that, but given the multiplexing model, and the fact that individual Http2Stream objects have their own timeouts that are independent of the Http2Session and Socket, does that existing model still make sense here?

[mcollina]

Landed as 3904d7c