node
node copied to clipboard
nodejs
HTTP2 `res.writableFinished` without calling `.end()`
I noticed this from an assertion i production. A http2 response can become writableEnded and writableFinished without the user ever calling response.end(). I believe there is some special logic for HEAD requests that causes this behavior.
I think this is suboptimal from a streams perspective but could still be considered ok behavior. I don't think "self ending" streams are a good idea. However, from a http1 compat perspective it does differ in behavior from http1 response which can cause unexpected problems if one assumes that a http2 compat response behaves exactly like a http1 response.
I basically had something like this in our code:
function onUpstreamComplete (trailers) {
const { res } = this
// Ensure we don't have a bug somewhere "corrupting" state by prematurely ending or destroying response.
assert(!res.destroyed)
assert(!res.writableEnded)
assert(!res.writableFinished)
writeTrailers(res, trailers)
res.end()
}
I would rather see something like this in HTTP2Response.
_construct (callback) {
this.writable = false // writable semantics are still a bit ambigious, can a stream be !writable and still be ok to call `.end()` on?
callback()
}
_write (buf, encoding, callback) {
if (this.method === 'HEAD') return callback(new SomeError())
...
}
To enforce that we don't write to HEAD requests. Right now we just quietly swallow it. Could do the same for http1.
Have you identified where this HEAD logic lives? We don't do this in HTTP/1, I think we should remove it.
I don't understand HTTP2 very well, but combing the code I found a few references where a head request prompted a writable stream close.
https://github.com/nodejs/node/blob/master/lib/internal/http2/core.js#L2682 https://github.com/nodejs/node/blob/master/lib/internal/http2/core.js#L2753
With http2 (and http3) it is possible for an HTTP request stream to be immediately closed for writing before the usercode had a chance to do anything with it. For http2, the duplex writable side is closed immediately on creation.
I think preferably the http compat should be fixed but I don't care so much about it. We can leave it as is IMO. I don't know of anyone else having had issues with it.
bad part is if stream is finished before res.end(cb), the cb is never called. in my case which is used as a promise resolver with await, the execution halts at that point which causes leak. I think even if stream is ended before end the cb should be called immediately.
hi @ronag is this issue still open and if yes can I help with it? I am a NodeJS developer. This will be my first ever contribution to open source and would require a lilttle help here to understand what is needed to solve this.
@AishwaryaChat I believe HTTP2 has bigger problems which. https://github.com/nodejs/node/issues/42710 https://github.com/nodejs/node/issues/44523 I See leaking streams even for simplest form of responses. That may cause this too.
Hello, has anyone fixed or submitted a pull request to fix this issue yet? If not, then I can resolve it.
go for it!
Can you guide me through, solving the issue I am very new to open source. I want to start contributing to the projects and want to get help from you !
function onUpstreamComplete(trailers) { const { res } = this;
// Ensure we don't have a bug somewhere "corrupting" state by prematurely ending or destroying the response. assert(!res.destroyed); assert(!res.writableEnded); assert(!res.writableFinished);
writeTrailers(res, trailers);
// Explicitly call res.end() for both regular and HEAD requests. if (!res.writableEnded) { res.end(); } } Hopefully it would work..
