Node: HTTP2 compatibility layer issues with invalid streams

[StephenLynx]

https://pastebin.com/LAfzJnQf
Node 12.10, using ssl. The whole code handles both http1 and http2 exactly the same, yet this only happens for ssl using http2.

[mscdex]

It's better to post the details here instead of a link in case the link ever dies.

Here's the text from the link:

c/home/LynxChan/src/be/node_modules/mongodb/lib/utils.js:132
      throw err;
      ^

Error [ERR_HTTP2_INVALID_STREAM]: The stream has been destroyed
    at Http2ServerResponse.write (internal/http2/compat.js:637:19)
    at GridFSBucketReadStream.<anonymous> (/home/LynxChan/src/be/engine/gridFsHandler.js:304:9)
    at GridFSBucketReadStream.emit (events.js:209:13)
    at addChunk (_stream_readable.js:305:12)
    at readableAddChunk (_stream_readable.js:286:11)
    at GridFSBucketReadStream.Readable.push (_stream_readable.js:220:10)
    at /home/LynxChan/src/be/node_modules/mongodb/lib/gridfs-stream/download.js:250:11
    at /home/LynxChan/src/be/node_modules/mongodb/lib/utils.js:414:17
    at executeCallback (/home/LynxChan/src/be/node_modules/mongodb/lib/utils.js:406:9)
    at handleCallback (/home/LynxChan/src/be/node_modules/mongodb/lib/utils.js:128:55)
Server worker 1 crashed.
Issues were found  with the templates.

Page boardsPage
Error, missing element linkOverboard
Error, missing element linkSfwOver
Worker 2 booted at Wed, 11 Sep 2019 18:14:54 GMT
/home/LynxChan/src/be/node_modules/mongodb/lib/utils.js:132
      throw err;
      ^

Error [ERR_HTTP2_INVALID_STREAM]: The stream has been destroyed
    at Http2ServerResponse.write (internal/http2/compat.js:637:19)
    at GridFSBucketReadStream.<anonymous> (/home/LynxChan/src/be/engine/gridFsHandler.js:304:9)
    at GridFSBucketReadStream.emit (events.js:209:13)
    at addChunk (_stream_readable.js:305:12)
    at readableAddChunk (_stream_readable.js:286:11)
    at GridFSBucketReadStream.Readable.push (_stream_readable.js:220:10)
    at /home/LynxChan/src/be/node_modules/mongodb/lib/gridfs-stream/download.js:250:11
    at /home/LynxChan/src/be/node_modules/mongodb/lib/utils.js:414:17
    at executeCallback (/home/LynxChan/src/be/node_modules/mongodb/lib/utils.js:406:9)
    at handleCallback (/home/LynxChan/src/be/node_modules/mongodb/lib/utils.js:128:55)
Server worker 2 crashed.

[jasnell]

Hmmm yeah, the http2 streams are handling things a bit more strict than the http1 code. We likely need to relax that in the compatibility layer. /cc @nodejs/http2 @apapirovski

[jasnell]

Actually, taking the confirmed bug label back off for the time being, want to test a bit more. @StephenLynx can you provide a simple reproduceable test case we can use that demonstrates the problem

[StephenLynx]

@jasnell Nope. All I got is a report from a person running my software. The information I got is that it happens every now and then after ssl is enabled. Is not the first person to report something similar. My software uses http2 for ssl, I don't run ssl on my own site. Most people end up letting nginx handle ssl, so is not often that people run into issues.
My software is gitgud.io/LynxChan/LynxChan. The part that touches http2 is https://gitgud.io/LynxChan/LynxChan/blob/2.2.x/src/be/workerBoot.js#L78

[sogaani]

This issue might be the same I reported here(second possible reason) and I wrote a reproduceable test case.

[StephenLynx]

@jasnell can you put back the confirmed bug tag? sogaani just posted a reproduceable test case.

[sogaani]

@StephenLynx
To be sure that the issue you reported and mine are the same, could you check if your software returns 304 or 204 when errors happen?

[StephenLynx]

@sogaani no, my software just crashes. I pretty much implemented a framework inside the software and I don't take in account the exception thrown by the compatibility layer, so it just crashes.

[zandaqo]

I also have an issue with 12.10 when I use HTTP2+SSL in production: every now and then the POST request's stream fails to fire end event and just hangs there waiting for more data. It happens unpredictably, no errors are thrown, so I cannot pin point the issue at the moment, but I do know that the stream fires data events and receive all the data but doesn't fire end, error, or frameError events to finish the request. As a workaround for now I just check in data if the received length is equal to the expected length and end the request myself.

[sogaani]

@StephenLynx Then I'm not sure your bug and reproduceable test are the same as mine.
Before exception,if your software calls res.writeHead(304) or res.writeHead(204), it might be the same.

[StephenLynx]

@sogaani No, the cause of the crash is a res.write with binary data.

[StephenLynx]

So, @jasnell what's up?

[mokimo]

Hmmm yeah, the http2 streams are handling things a bit more strict than the http1 code. We likely need to relax that in the compatibility layer.

Would it not make sense to make it behave equal to http1 if it's a compability layer to begin with? 😕 (Having some errors with the current behavior as well)

[StephenLynx]

Any news, @jasnell ?

[mokimo]

Btw, I tried (and failed) to reproduce this with a minimal setup/unit test so far, could you try to set something up? @StephenLynx

[StephenLynx]

@addaleax I don't know who else to mention here. What's the deal with this issue that no one seems to talk about? It's been confirmed it's just the compatibility layer being too strict. Is this being discussed? Did people just forget? Are people just too busy to do anything about it?

[ronag]

@StephenLynx: This looks more like a bug in http/1 that doesn't error if write is called after destroy (which is the "correct" behaviour for stream).

Though http/2 compat should error with ERR_STREAM_DESTROYED instead of ERR_HTTP2_INVALID_STREAM to fully follow the streams "spec".

[StephenLynx]

@ronag that would be a very, very, very, very old bug. Because I have used http1 the same way for almost 5 years without issues. I have a feeling if http1 didn't just ignore w/e error it is ignoring, it would be chaotic for developers. I never manually destroy the request when this error triggers, whatever happens, happens inside node's code. Are you sure you are correct?

[ronag]

that would be a very, very, very, very old bug.

Well, it might not have been a bug in the past but there has been breaking changes in the Node API over the years which might make some consider this a bug.

Are you sure you are correct?

http.ServerResponse is "supposed" to be a stream.Writable however due to performance reasons it implements the stream API itself without using the stream framework (this is something we/me hope to change in the future if we can get around the performance issue). But it should still act like a stream.Writable, which in this case I believe would error with ERR_STREAM_DESTROYED.

This would not be a problem if there was a 'error' listener on the instance, which you should probably have regardless.

Whether this should be fixed or not is I guess up for debate. My personal opinion is to keep things as consistent as possible with the "expected" behaviour of streams.

There are also other discrepancies that we should/might address that might be of interest https://github.com/nodejs/node/issues/29829.

I'm probably a bit more strict on the "correctness" vs "breaking" topic than most. @mcollina @jasnell What's your take here? Should we relax http/2 compat to match http/1, or tighten http/1 to match streams?

[ronag]

@Trott: I would recommend http and stream labels on this issue.

[StephenLynx]

@ronag I do have a callback on the error listener.
https://gitgud.io/LynxChan/LynxChan/blob/master/src/be/workerBoot.js#L92
Did I put it in the wrong place, did I get wrong what you meant or maybe it's not sending the error properly to the event?

[StephenLynx]

Also, as a user just expressing what I would like to have when using node as a tool, I'd REALLY love the http2/compat to be as permissive as http1 if the issue is http1 not strictly following specs.

[ronag]

@StephenLynx You need an 'error' listener on the res instance here, https://gitgud.io/LynxChan/LynxChan/blob/master/src/be/workerBoot.js#L87

I don't think the server 'error' listener does anything. You might want to listen to clientError on the server instance, but that will just catch certain errors (won't help in this case).

[ronag]

Also, as a side note, I'm not 100% this discrepancy is the issue.

If the response is destroyed both in the http/1 and http/2 compat case, then http/1 will be permissive and that's why you observe different behaviors.

If the problem is that the response is destroyed in the http/2 compat but not in the http/1 case then it's something else.

It's hard to tell which one of these is the actual root cause.

[StephenLynx]

I see. I'll add the error listener on res and report if anything unexpected happens. And yeah. The issue might not be a discrepancy in the behavior. I didn't try too hard to debug it so I can't tell for sure what the issue is, only that is happens exclusively on the compat layer.

[StephenLynx]

@ronag I had a guy that runs my software test it, It didn't seemed to have worked.
This is the crash he had:
https://pastebin.com/raw/5W9vJtfa
This is how he added a listener to error:
https://pastebin.com/raw/GBWMwvrU

Let me know if the event wasn't handled properly.

[ronag]

Oh yea, that is a bug.

@jasnell: The throw err case here https://github.com/nodejs/node/blob/master/lib/internal/http2/compat.js#L662 should probably be a destroy(err):

    if (this[kState].closed) {
      const err = new ERR_HTTP2_INVALID_STREAM();
      if (typeof cb === 'function')
        process.nextTick(cb, err);
      this.destroy(err);
      return;
    }

[StephenLynx]

Thrilled that solid progress was made. This issue have been a thorn on my side since before v12 came out. I finally added http2 on my software and people had to resort to nginx handling ssl because it would just crash. Now, after this is fixed, the error would be caught by listening to the error event or no error would happen at all? Because I never added a listener to errors on http1 and never had issues either.

[ronag]

@StephenLynx: FYI https://github.com/nodejs/node/pull/30964

[DullReferenceException]

I also have an issue with 12.10 when I use HTTP2+SSL in production: every now and then the POST request's stream fails to fire end event and just hangs there waiting for more data. It happens unpredictably, no errors are thrown, so I cannot pin point the issue at the moment, but I do know that the stream fires data events and receive all the data but doesn't fire end, error, or frameError events to finish the request. As a workaround for now I just check in data if the received length is equal to the expected length and end the request myself.

@zandaqo: I think your message got lost in this issue; it sounds like a different problem than what was originally posted. FWIW, we're having what I think is the exact same issue. I'm still searching to see if there's a different issue focusing on this problem. If you know of where that is, it'd be great to see your workaround.

[zandaqo]

@DullReferenceException I'm glad you brought it up, I checked recently and the issue is still present in Node 13. As I wrote before, my workaround is terminating requests when the received data length equals the Content-Length header. For raw-body it results in appending a single line to onData function used as a callback for data events:

  function onData(chunk) {
    if (complete) return;

    received += chunk.length;

    if (limit !== null && received > limit) {
      done(createError(413, 'request entity too large', {
        limit,
        received,
        type: 'entity.too.large',
      }));
    } else if (decoder) {
      buffer += decoder.write(chunk);
    } else {
      buffer.push(chunk);
    }

    if (received === length) onEnd();
  }