HTTP2 `res.writableFinished` without calling `.end()`

[ronag]

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()
} 

[ronag]

@mcollina thoughts?

[ronag]

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.

[ronag]

@nodejs/http

[mcollina]

Have you identified where this HEAD logic lives? We don't do this in HTTP/1, I think we should remove it.

[jonchurch]

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

[mcollina]

@jasnell do you remember why we did those checks for HEAD requests?

[jasnell]

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.

[mcollina]

@ronag how would you like to proceed with this?

[ronag]

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.

[rahbari]

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.