http2: fix missing 'timeout' event emit in request and response #20918

DaAitch · 2018-05-23T23:19:34Z

A timeout in a http2 stream should emit a 'timeout' event also on
request and response object. If there is no listener on stream,
request or response, stream should immediately be destroyed.

Fixes: #20079

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

A timeout in a http2 stream should emit a 'timeout' event also on request and response object. If there is no listener on stream, request or response, stream should immediately be destroyed. Fixes: nodejs#20079

apapirovski

Hi @DaAitch — thanks for working on this. I would say that this might be the slightly wrong approach. We could simplify this by just listening to the timeout even in req and res. From there we would have to make sure any logic matches what we have on the http side in terms of whether we destroy the stream/session. There are some "complexities" in how the http module handles timeouts in relation to whether the req & res are complete or not.

I'm currently in the midst of travelling but would be happy to provide more info when I have a bit more time.

DaAitch · 2018-05-25T16:03:06Z

We could simplify this by just listening to the timeout even in req and res

I have also investigated on that, but in this case, we cannot find out, if there is no listener to timeout at req and res to destroy the stream. I also read the http module, and they're also not listening on the events, but doing it like

const a = req.emit('timeout');
const b = res.emit('timeout');

if (!a && !b) {
  destroy();
}

mcollina · 2018-05-30T05:06:25Z

test/parallel/test-http2-compat-serverrequest-settimeout.js

@@ -12,6 +12,10 @@ server.on('request', (req, res) => {
  req.setTimeout(msecs, common.mustCall(() => {
    res.end();
  }));
+
+  res.on('timeout', common.mustCall());
+  req.on('timeout', common.mustCall());


Can you please add some assertions that verifies that 'timeout' is not emitted?

@mcollina sure I can. You mean between the setTimeout call and its callback, there should be no timeout events on req and res?

mcollina · 2018-05-30T05:08:09Z

lib/internal/http2/core.js

+      if (component.emit('timeout')) {
+        hasTimeoutCallback = true;
+      }
+    }


I would just check if if(!this.emit('timeout')) { this.session.destroy() }. It would need a test on the change in http2 core behavior.

@mcollina I think, this is not what the docs are saying. We have 3 EventEmitters (req, res, stream) and if all of them return false, which means "there is no timeout listener" (according to the docs), then we can destroy the session.

mcollina · 2018-05-30T05:09:14Z

lib/internal/http2/compat.js

                        stream, headers, flags, rawHeaders) {
  const server = this;
  const request = new ServerRequest(stream, headers, undefined, rawHeaders);
  const response = new ServerResponse(stream);

+  stream[kStreamEventsComposite].push(request);
+  stream[kStreamEventsComposite].push(response);


Instead of adding things to an array, I would add an event listener to 'timeout' on the stream.

@mcollina as I described in the 2nd comment here, if we add a timeout listener to an EventEmitter, the checks with if (...emit(...)) will not work, because they all are always true then.

apapirovski · 2018-05-30T05:36:10Z

I'm still thinking about this. Hopefully can provide more insight after the summit.

jasnell · 2018-09-10T18:16:20Z

An alternative fix landed in #22252

http2: fix missing 'timeout' event emit in request and response

8eabb3f

A timeout in a http2 stream should emit a 'timeout' event also on request and response object. If there is no listener on stream, request or response, stream should immediately be destroyed. Fixes: nodejs#20079

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label May 23, 2018

apapirovski reviewed May 25, 2018

View reviewed changes

BridgeAR requested review from apapirovski, mcollina and jasnell May 28, 2018 13:34

mcollina reviewed May 30, 2018

View reviewed changes

jasnell mentioned this pull request Aug 10, 2018

http2: emit timeout on compat request and response #22252

Closed

3 tasks

jasnell closed this Sep 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http2: fix missing 'timeout' event emit in request and response #20918

http2: fix missing 'timeout' event emit in request and response #20918

DaAitch commented May 23, 2018

apapirovski left a comment

DaAitch commented May 25, 2018 •

edited

Loading

mcollina May 30, 2018

DaAitch May 31, 2018

mcollina May 30, 2018

DaAitch May 31, 2018 •

edited

Loading

mcollina May 30, 2018

DaAitch May 31, 2018

apapirovski commented May 30, 2018

jasnell commented Sep 10, 2018

http2: fix missing 'timeout' event emit in request and response #20918

http2: fix missing 'timeout' event emit in request and response #20918

Conversation

DaAitch commented May 23, 2018

Checklist

apapirovski left a comment

Choose a reason for hiding this comment

DaAitch commented May 25, 2018 • edited Loading

mcollina May 30, 2018

Choose a reason for hiding this comment

DaAitch May 31, 2018

Choose a reason for hiding this comment

mcollina May 30, 2018

Choose a reason for hiding this comment

DaAitch May 31, 2018 • edited Loading

Choose a reason for hiding this comment

mcollina May 30, 2018

Choose a reason for hiding this comment

DaAitch May 31, 2018

Choose a reason for hiding this comment

apapirovski commented May 30, 2018

jasnell commented Sep 10, 2018

DaAitch commented May 25, 2018 •

edited

Loading

DaAitch May 31, 2018 •

edited

Loading