Repository navigation
Problems with popular middleware #145
Description
Activity
Very likely
on-finishedorsendwould need to be updated to support HTTP2.
Specificallyon-finishedis very HTTP1 specific where a single socket is attached to a single HTTP request.@sebdeckers @jasnell maybe we need to alias
connectionandsocketto 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 Are you running with the patch from #130 ? This should provide the
req.socketproperty and allowon-finishedto 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
isFinishedcode 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
Reacted by Sebastiaan Deckerson-finished is looking for a socket on the response, not the request.
@sebdeckers yes, I think so. However I'm starting to think we should expose the HTTP2Stream as
socketandconnection(making them an alias forstream), rather than exposing the actual socket. What do you think?@mcollina Hmm, not sure I understand the reasoning for that, could you show how that is useful? (I can see how H2
streammight be conceptually analogous to H1socket, 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-finishedrely 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
encryptedandremoteAddressare easy properties to add, and maybe evensetTimeout(nghttp2 does not have the concept, we would have to implement this on our side).So
req|res.socketwould 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.Reacted by Sebastiaan Deckers
In #126 I briefly refer to a problem with using the
serve-staticmodule. I have now tracked down what the issue is.serve-staticusessendwhich in turn useson-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
compressionmodule. 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