Problems with popular middleware

In #126 I briefly refer to a problem with using the serve-static module. I have now tracked down what the issue is.

serve-static uses send which in turn uses on-finished. This seems to assume that the response object will either already have a socket attached on with emit the 'socket' event when one is attached.

This never happens and so it can never finish the connection

I also have just started using the compression module. This also fails because under the hood its calling _implicitHeader() which doesn't exist on this implementation.

I am not sure either of these two issues are ones with this module, but I am raising a heads up because obviously when this goes lives some people will trip over them

@sebdeckers

Very likely on-finished or send would need to be updated to support HTTP2.
Specifically on-finished is very HTTP1 specific where a single socket is attached to a single HTTP request.

@sebdeckers @jasnell maybe we need to alias connection and socket to the underlining http2 stream in the compatibility layer. What do you think?
Specifically, https://github.com/jshttp/on-finished/blob/master/index.js#L113-L114 waits for that duplex to end.

@akc42

@akc42 Are you running with the patch from #130 ? This should provide the req.socket property and allow on-finished to add its event handlers. Could you perhaps share a minimal example of this problem? I'd love to take a closer look.

@mcollina Not sure about changing socket to a stream; wouldn't that break expected behaviour like what we saw in modules that look at socket properties for crypto settings and such?

FWIW the isFinished code won't work with the getter we are using.

  if (typeof msg.finished === 'boolean') {

https://github.com/jshttp/on-finished/blob/master/index.js#L68

get finished() {

https://github.com/nodejs/http2/blob/master/lib/internal/http2/compat.js#L270

This will always return undefined.

I think I am running with the very latest. fetched, merged and compiled this morning

on-finished is looking for a socket on the response, not the request.

@akc42

@akc42 Oh, I see. That's not a documented API though AFAIK. @mcollina Is this an oversight in the http/1 documentation? Should we add a socket/connection to the response for backwards compatibility?

@sebdeckers

@sebdeckers yes, I think so. However I'm starting to think we should expose the HTTP2Stream as socket and connection (making them an alias for stream), rather than exposing the actual socket. What do you think?

@mcollina

@mcollina Hmm, not sure I understand the reasoning for that, could you show how that is useful? (I can see how H2 stream might be conceptually analogous to H1 socket, but is not very important to the compatibility layer.

Looking again at the samples discovered in #130, I'm worried it would break the following:

.connection.encrypted
.connection.remoteAddress
.socket.setTimeout(...)

Though this might still work:

.socket.destroy()

All the stream events for on-finished rely on the fact that the socket is 1-1 with the request. This is a given assumption throughout the whole API, and I think we can't achieve that with the underlining socket.

Adding properties/getters is relatively easy, as encrypted and remoteAddress are easy properties to add, and maybe even setTimeout (nghttp2 does not have the concept, we would have to implement this on our side).

So req|res.socket would be a hybrid of the TCP socket and the H2 stream. 🤔 Yeah that makes sense. We can put that together with Object.assign or a Proxy/Reflect Frankenstein. 😂

I'm actually thinking of adding a bunch of getters to our H2 stream (which is already a Duplex). I think that might be the best analogy.

Navigation Menu

Repository navigation

Problems with popular middleware #145

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Problems with popular middleware #145

Description

Activity

mcollina commented on May 21, 2017

sebdeckers commented on May 21, 2017

sebdeckers commented on May 21, 2017

akc42 commented on May 21, 2017

akc42 commented on May 21, 2017

sebdeckers commented on May 21, 2017

mcollina commented on May 22, 2017

sebdeckers commented on May 22, 2017

mcollina commented on May 22, 2017

sebdeckers commented on May 22, 2017

mcollina commented on May 22, 2017

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions