HTTP2 compatibility layer issues with invalid streams #29529

StephenLynx · 2019-09-11T19:42:17Z

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 · 2019-09-11T21:37:13Z

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 · 2019-09-11T22:20:15Z

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 · 2019-09-11T22:21:44Z

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 · 2019-09-12T02:16:31Z

@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 · 2019-09-12T02:54:43Z

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

StephenLynx · 2019-09-14T16:53:55Z

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

sogaani · 2019-09-15T10:10:19Z

@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 · 2019-09-15T12:33:23Z

@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 · 2019-09-16T20:36:51Z

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 · 2019-09-17T03:55:55Z

@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 · 2019-09-17T04:30:11Z

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

StephenLynx · 2019-10-01T22:32:41Z

So, @jasnell what's up?

mokimo · 2019-11-13T14:09:40Z

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 · 2019-11-19T12:58:44Z

Any news, @jasnell ?

mokimo · 2019-11-19T13:14:18Z

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 · 2019-12-13T15:28:13Z

@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 · 2019-12-13T23:16:22Z

@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 · 2019-12-14T16:13:49Z

@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 · 2019-12-14T16:52:46Z

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 #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 · 2019-12-14T17:05:59Z

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

StephenLynx · 2019-12-14T17:06:32Z

@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 · 2019-12-14T17:12:55Z

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 · 2019-12-14T17:13:02Z

@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 · 2019-12-14T17:15:09Z

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 · 2019-12-14T17:17:44Z

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 · 2019-12-14T17:47:05Z

@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 · 2019-12-14T17:51:22Z

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 · 2019-12-14T17:54:15Z

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 · 2019-12-14T18:12:17Z

@StephenLynx: FYI #30964

HTTP2ServerResponse.write would behave differently than both http1 and streams. This PR makes it more compliant with stream.Writable behaviour. Refs: nodejs#29529

HTTP2ServerResponse.write would behave differently than both http1 and streams. This PR makes it more compliant with stream.Writable behaviour. PR-URL: #30964 Refs: #29529 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>

DullReferenceException · 2020-01-10T19:19:29Z

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 · 2020-01-12T10:44:38Z

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

HTTP2ServerResponse.write would behave differently than both http1 and streams. This PR makes it more compliant with stream.Writable behaviour. PR-URL: nodejs#30964 Refs: nodejs#29529

HTTP2ServerResponse.write would behave differently than both http1 and streams. This PR makes it more compliant with stream.Writable behaviour. PR-URL: nodejs#30964 Refs: nodejs#29529 Backport-PR-URL: nodejs#31444

HTTP2ServerResponse.write would behave differently than both http1 and streams. This PR makes it more compliant with stream.Writable behaviour. Backport-PR-URL: #31444 PR-URL: #30964 Refs: #29529 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>

jasnell added confirmed-bug Issues with confirmed bugs. http2 Issues or PRs related to the http2 subsystem. and removed confirmed-bug Issues with confirmed bugs. labels Sep 11, 2019

ronag mentioned this issue Dec 14, 2019

The differences between http2 compatibility API and http[s] #29829

Open

32 tasks

ronag mentioned this issue Dec 14, 2019

http2: make http2/compat.write more http/1 compliant #30964

Closed

4 tasks

Trott added http Issues or PRs related to the http subsystem. stream Issues and PRs related to the stream subsystem. labels Dec 15, 2019

asilvas mentioned this issue Jan 10, 2020

[http2] Force complete if all data received stream-utils/raw-body#68

Closed

DullReferenceException mentioned this issue Jan 11, 2020

Request streams from http2 intermittantly not emitting end event #31309

Closed

sxa mentioned this issue Jan 21, 2020

[v12.x backport] http2: make HTTP2ServerResponse more streams compliant #31444

Closed

4 tasks

Mesteery closed this as completed Feb 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTTP2 compatibility layer issues with invalid streams #29529

HTTP2 compatibility layer issues with invalid streams #29529

StephenLynx commented Sep 11, 2019

mscdex commented Sep 11, 2019

jasnell commented Sep 11, 2019

jasnell commented Sep 11, 2019

StephenLynx commented Sep 12, 2019

sogaani commented Sep 12, 2019 •

edited

StephenLynx commented Sep 14, 2019

sogaani commented Sep 15, 2019

StephenLynx commented Sep 15, 2019

zandaqo commented Sep 16, 2019

sogaani commented Sep 17, 2019

StephenLynx commented Sep 17, 2019

StephenLynx commented Oct 1, 2019

mokimo commented Nov 13, 2019 •

edited

StephenLynx commented Nov 19, 2019

mokimo commented Nov 19, 2019

StephenLynx commented Dec 13, 2019

ronag commented Dec 13, 2019 •

edited

StephenLynx commented Dec 14, 2019

ronag commented Dec 14, 2019 •

edited

ronag commented Dec 14, 2019

StephenLynx commented Dec 14, 2019

StephenLynx commented Dec 14, 2019

ronag commented Dec 14, 2019 •

edited

ronag commented Dec 14, 2019 •

edited

StephenLynx commented Dec 14, 2019

StephenLynx commented Dec 14, 2019 •

edited

ronag commented Dec 14, 2019 •

edited

StephenLynx commented Dec 14, 2019

ronag commented Dec 14, 2019

DullReferenceException commented Jan 10, 2020

zandaqo commented Jan 12, 2020

HTTP2 compatibility layer issues with invalid streams #29529

HTTP2 compatibility layer issues with invalid streams #29529

Comments

StephenLynx commented Sep 11, 2019

mscdex commented Sep 11, 2019

jasnell commented Sep 11, 2019

jasnell commented Sep 11, 2019

StephenLynx commented Sep 12, 2019

sogaani commented Sep 12, 2019 • edited

StephenLynx commented Sep 14, 2019

sogaani commented Sep 15, 2019

StephenLynx commented Sep 15, 2019

zandaqo commented Sep 16, 2019

sogaani commented Sep 17, 2019

StephenLynx commented Sep 17, 2019

StephenLynx commented Oct 1, 2019

mokimo commented Nov 13, 2019 • edited

StephenLynx commented Nov 19, 2019

mokimo commented Nov 19, 2019

StephenLynx commented Dec 13, 2019

ronag commented Dec 13, 2019 • edited

StephenLynx commented Dec 14, 2019

ronag commented Dec 14, 2019 • edited

ronag commented Dec 14, 2019

StephenLynx commented Dec 14, 2019

StephenLynx commented Dec 14, 2019

ronag commented Dec 14, 2019 • edited

ronag commented Dec 14, 2019 • edited

StephenLynx commented Dec 14, 2019

StephenLynx commented Dec 14, 2019 • edited

ronag commented Dec 14, 2019 • edited

StephenLynx commented Dec 14, 2019

ronag commented Dec 14, 2019

DullReferenceException commented Jan 10, 2020

zandaqo commented Jan 12, 2020

sogaani commented Sep 12, 2019 •

edited

mokimo commented Nov 13, 2019 •

edited

ronag commented Dec 13, 2019 •

edited

ronag commented Dec 14, 2019 •

edited

ronag commented Dec 14, 2019 •

edited

ronag commented Dec 14, 2019 •

edited

StephenLynx commented Dec 14, 2019 •

edited

ronag commented Dec 14, 2019 •

edited