http: fix incorrect headersTimeout measurement (alt) #32329

OrKoN · 2020-03-17T17:52:51Z

For keep-alive connections, the headersTimeout may fire during
subsequent request because the measurement was reset after
a request and not before a request.

Fixes: #27363

Alternative implementation for: #30184

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

For keep-alive connections, the headersTimeout may fire during subsequent request because the measurement was reset after a request and not before a request. Fixes: nodejs#27363

OrKoN · 2020-03-17T17:53:21Z

lib/_http_client.js

@@ -727,7 +727,8 @@ function tickOnSocket(req, socket) {
                    new HTTPClientAsyncResource('HTTPINCOMINGMESSAGE', req),
                    req.maxHeaderSize || 0,
                    req.insecureHTTPParser === undefined ?
-                      isLenient() : req.insecureHTTPParser);
+                      isLenient() : req.insecureHTTPParser,
+                    0);


for the response parsing by the client the headers timeout is not relevant

OrKoN · 2020-03-17T17:54:09Z

lib/_http_server.js

-    return;
-  }
+function onParserTimeout(server, socket) {
+  const serverTimeout = server.emit('timeout', socket);


time tracking is moved to c++ code

OrKoN · 2020-03-17T17:54:52Z

src/node_http_parser.cc

+        if (!cb->IsFunction())
+          return;
+
+        MakeCallback(cb.As<Function>(), 0, nullptr);


should I check the return value? anything else needed to mark the parsing error?

@OrKoN Since you’d be return;ing here either way, whether the call threw an exception or not, I don’t think so. You can wrap the call in USE() to make it explicit that you’re discarding the return value because of that.

OrKoN · 2020-03-17T17:58:20Z

src/node_http_parser.cc

+      auto parsing_time = (now - header_parsing_start_time_) / 1e6;
+
+      if (parsing_time > headers_timeout_) {
+        Local<Value> cb =


is there a good way to avoid having an extra callback here?

@OrKoN I don’t think so. But if it only fires in the timeout case, that should be okay.

OrKoN · 2020-03-17T19:06:43Z

There is one test case failing:

=== release test-graph.http ===
Path: async-hooks/test-graph.http
--- stderr ---
'timeout:1' expected to be triggered by 'tcp:2', but was triggered by 'httpincomingmessage:1' instead.
assert.js:102
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

1 !== 0

    at verifyGraph (/home/runner/work/node/node/test/async-hooks/verify-graph.js:100:10)
    at process.<anonymous> (/home/runner/work/node/node/test/async-hooks/test-graph.http.js:29:3)
    at process.emit (events.js:327:22) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: 1,
  expected: 0,
  operator: 'strictEqual'
}
Command: out/Release/node /home/runner/work/node/node/test/async-hooks/test-graph.http.js

I am not familiar with async hooks implementation and if anyone can suggest what's wrong here, I'd really appreciate.

OrKoN · 2020-03-19T14:42:32Z

I think the difference in the async-hooks test is caused by the call to nowDate which was setting a timer and which is now removed and the internal caching timer is triggered by another call (probably, utcDate).

mcollina · 2020-03-19T15:44:10Z

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/543/

mcollina · 2020-03-19T22:43:52Z

Benchmark CI looks ok to me

``` confidence improvement accuracy (*) (**) (***) 23:20:26 http/bench-parser.js n=100000 len=16 * 2.15 % ±1.64% ±2.19% ±2.86% 23:20:26 http/bench-parser.js n=100000 len=32 1.51 % ±2.93% ±3.90% ±5.08% 23:20:26 http/bench-parser.js n=100000 len=4 0.95 % ±3.47% ±4.67% ±6.17% 23:20:26 http/bench-parser.js n=100000 len=8 5.72 % ±6.32% ±8.50% ±11.23% 23:20:26 http/check_invalid_header_char.js n=1000000 input='' -1.69 % ±4.17% ±5.55% ±7.23% 23:20:26 http/check_invalid_header_char.js n=1000000 input='1' -4.21 % ±4.76% ±6.37% ±8.37% 23:20:26 http/check_invalid_header_char.js n=1000000 input='20091' * -3.33 % ±3.01% ±4.00% ±5.21% 23:20:26 http/check_invalid_header_char.js n=1000000 input='close' ** -5.49 % ±3.15% ±4.21% ±5.51% 23:20:26 http/check_invalid_header_char.js n=1000000 input='en-US' -2.43 % ±4.33% ±5.77% ±7.51% 23:20:26 http/check_invalid_header_char.js n=1000000 input='foo\\nbar' 0.34 % ±4.23% ±5.63% ±7.33% 23:20:26 http/check_invalid_header_char.js n=1000000 input='group_acmeair' 1.03 % ±2.87% ±3.82% ±4.97% 23:20:26 http/check_invalid_header_char.js n=1000000 input='gzip' -2.76 % ±4.15% ±5.53% ±7.20% 23:20:26 http/check_invalid_header_char.js n=1000000 input='keep-alive' 0.12 % ±3.33% ±4.44% ±5.77% 23:20:26 http/check_invalid_header_char.js n=1000000 input='LONG_AND_INVALID' -1.62 % ±1.80% ±2.39% ±3.12% 23:20:26 http/check_invalid_header_char.js n=1000000 input='private' -3.55 % ±4.37% ±5.82% ±7.59% 23:20:26 http/check_invalid_header_char.js n=1000000 input='SAMEORIGIN' * -4.27 % ±3.66% ±4.89% ±6.39% 23:20:26 http/check_invalid_header_char.js n=1000000 input='Sat, 07 May 2016 16:54:48 GMT' -2.39 % ±2.54% ±3.38% ±4.40% 23:20:26 http/check_invalid_header_char.js n=1000000 input='text/html; charset=utf-8' 1.63 % ±3.17% ±4.24% ±5.57% 23:20:26 http/check_invalid_header_char.js n=1000000 input='text/plain' * -3.60 % ±3.36% ±4.48% ±5.86% 23:20:26 http/check_invalid_header_char.js n=1000000 input='\\t\\t\\t\\t\\t\\t\\t\\t\\t\\tFoo bar baz' * -4.19 % ±3.20% ±4.28% ±5.62% 23:20:26 http/check_invalid_header_char.js n=1000000 input='\\x7F' -1.18 % ±4.16% ±5.55% ±7.25% 23:20:26 http/check_invalid_header_char.js n=1000000 input='中文呢' 0.66 % ±4.67% ±6.22% ±8.10% 23:20:26 http/check_is_http_token.js n=1000000 key=':' -1.89 % ±6.70% ±8.92% ±11.62% 23:20:26 http/check_is_http_token.js n=1000000 key='((((())))' -2.34 % ±6.06% ±8.07% ±10.53% 23:20:26 http/check_is_http_token.js n=1000000 key='@@' -0.81 % ±6.30% ±8.39% ±10.92% 23:20:26 http/check_is_http_token.js n=1000000 key='Accept-Ranges' -0.88 % ±2.90% ±3.86% ±5.02% 23:20:26 http/check_is_http_token.js n=1000000 key=':alternate-protocol' 0.62 % ±7.26% ±9.66% ±12.58% 23:20:26 http/check_is_http_token.js n=1000000 key='alternate-protocol:' -3.34 % ±4.01% ±5.33% ±6.94% 23:20:26 http/check_is_http_token.js n=1000000 key='alternate-protocol' -2.35 % ±5.15% ±6.85% ±8.91% 23:20:26 http/check_is_http_token.js n=1000000 key='alt-svc' 0.73 % ±5.97% ±7.95% ±10.35% 23:20:26 http/check_is_http_token.js n=1000000 key='Cache-Control' 1.64 % ±3.24% ±4.32% ±5.64% 23:20:26 http/check_is_http_token.js n=1000000 key='Connection' -0.01 % ±2.94% ±3.91% ±5.09% 23:20:26 http/check_is_http_token.js n=1000000 key='Content-Encoding' -1.71 % ±4.07% ±5.42% ±7.07% 23:20:26 http/check_is_http_token.js n=1000000 key='content-length' -0.18 % ±2.90% ±3.86% ±5.03% 23:20:26 http/check_is_http_token.js n=1000000 key='Content-Location' -2.30 % ±4.64% ±6.17% ±8.03% 23:20:26 http/check_is_http_token.js n=1000000 key='content-type' 0.61 % ±3.50% ±4.66% ±6.07% 23:20:26 http/check_is_http_token.js n=1000000 key='Content-Type' 7.05 % ±8.46% ±11.31% ±14.82% 23:20:26 http/check_is_http_token.js n=1000000 key='date' -0.27 % ±2.92% ±3.88% ±5.05% 23:20:26 http/check_is_http_token.js n=1000000 key='ETag' 4.21 % ±5.85% ±7.82% ±10.24% 23:20:26 http/check_is_http_token.js n=1000000 key='Expires' 0.90 % ±3.20% ±4.26% ±5.56% 23:20:26 http/check_is_http_token.js n=1000000 key='Keep-Alive' 1.57 % ±3.52% ±4.69% ±6.10% 23:20:26 http/check_is_http_token.js n=1000000 key='Last-Modified' -1.16 % ±4.07% ±5.42% ±7.05% 23:20:26 http/check_is_http_token.js n=1000000 key='location' -3.06 % ±4.15% ±5.52% ±7.19% 23:20:26 http/check_is_http_token.js n=1000000 key='server' 2.93 % ±4.61% ±6.15% ±8.03% 23:20:26 http/check_is_http_token.js n=1000000 key='Server' 1.74 % ±3.64% ±4.85% ±6.31% 23:20:26 http/check_is_http_token.js n=1000000 key='status' 1.05 % ±3.68% ±4.89% ±6.37% 23:20:26 http/check_is_http_token.js n=1000000 key='TCN' 4.20 % ±5.12% ±6.82% ±8.88% 23:20:26 http/check_is_http_token.js n=1000000 key='Transfer-Encoding' -1.33 % ±4.85% ±6.46% ±8.41% 23:20:26 http/check_is_http_token.js n=1000000 key='Vary' -0.23 % ±4.29% ±5.71% ±7.44% 23:20:26 http/check_is_http_token.js n=1000000 key='version' -0.26 % ±3.68% ±4.90% ±6.38% 23:20:26 http/check_is_http_token.js n=1000000 key='x-frame-options' 0.57 % ±2.54% ±3.38% ±4.40% 23:20:26 http/check_is_http_token.js n=1000000 key='x-xss-protection' -1.27 % ±4.81% ±6.40% ±8.33% 23:20:26 http/check_is_http_token.js n=1000000 key='中文呢' -3.24 % ±6.48% ±8.62% ±11.24% 23:20:26 http/chunked.js duration=5 c=100 len=1 n=16 benchmarker='wrk' 0.45 % ±0.47% ±0.63% ±0.82% 23:20:26 http/chunked.js duration=5 c=100 len=1 n=1 benchmarker='wrk' 0.04 % ±0.07% ±0.09% ±0.11% 23:20:26 http/chunked.js duration=5 c=100 len=1 n=4 benchmarker='wrk' 0.12 % ±0.15% ±0.20% ±0.26% 23:20:26 http/chunked.js duration=5 c=100 len=1 n=8 benchmarker='wrk' 0.11 % ±0.23% ±0.31% ±0.41% 23:20:26 http/chunked.js duration=5 c=100 len=256 n=16 benchmarker='wrk' ** 1.07 % ±0.70% ±0.93% ±1.22% 23:20:26 http/chunked.js duration=5 c=100 len=256 n=1 benchmarker='wrk' 0.01 % ±0.05% ±0.07% ±0.09% 23:20:26 http/chunked.js duration=5 c=100 len=256 n=4 benchmarker='wrk' 0.03 % ±0.14% ±0.18% ±0.24% 23:20:26 http/chunked.js duration=5 c=100 len=256 n=8 benchmarker='wrk' 0.16 % ±0.25% ±0.33% ±0.43% 23:20:26 http/chunked.js duration=5 c=100 len=64 n=16 benchmarker='wrk' 0.78 % ±0.86% ±1.14% ±1.49% 23:20:26 http/chunked.js duration=5 c=100 len=64 n=1 benchmarker='wrk' 0.00 % ±0.08% ±0.10% ±0.13% 23:20:26 http/chunked.js duration=5 c=100 len=64 n=4 benchmarker='wrk' 0.06 % ±0.12% ±0.15% ±0.20% 23:20:26 http/chunked.js duration=5 c=100 len=64 n=8 benchmarker='wrk' -0.02 % ±0.26% ±0.35% ±0.46% 23:20:26 http/client-request-body.js method='end' len=1024 type='asc' dur=5 -0.28 % ±7.69% ±10.25% ±13.38% 23:20:26 http/client-request-body.js method='end' len=1024 type='buf' dur=5 0.63 % ±8.70% ±11.58% ±15.07% 23:20:26 http/client-request-body.js method='end' len=1024 type='utf' dur=5 -1.36 % ±7.72% ±10.28% ±13.39% 23:20:26 http/client-request-body.js method='end' len=256 type='asc' dur=5 -3.93 % ±8.14% ±10.83% ±14.11% 23:20:26 http/client-request-body.js method='end' len=256 type='buf' dur=5 4.35 % ±9.08% ±12.08% ±15.73% 23:20:26 http/client-request-body.js method='end' len=256 type='utf' dur=5 4.40 % ±8.27% ±11.00% ±14.33% 23:20:26 http/client-request-body.js method='end' len=32 type='asc' dur=5 1.28 % ±8.64% ±11.50% ±14.98% 23:20:26 http/client-request-body.js method='end' len=32 type='buf' dur=5 2.35 % ±9.70% ±12.90% ±16.79% 23:20:26 http/client-request-body.js method='end' len=32 type='utf' dur=5 0.75 % ±9.55% ±12.71% ±16.56% 23:20:26 http/client-request-body.js method='write' len=1024 type='asc' dur=5 -0.52 % ±8.16% ±10.86% ±14.14% 23:20:26 http/client-request-body.js method='write' len=1024 type='buf' dur=5 3.41 % ±8.53% ±11.35% ±14.78% 23:20:26 http/client-request-body.js method='write' len=1024 type='utf' dur=5 1.03 % ±8.23% ±10.94% ±14.24% 23:20:26 http/client-request-body.js method='write' len=256 type='asc' dur=5 2.44 % ±10.11% ±13.46% ±17.52% 23:20:26 http/client-request-body.js method='write' len=256 type='buf' dur=5 -0.57 % ±8.72% ±11.60% ±15.09% 23:20:26 http/client-request-body.js method='write' len=256 type='utf' dur=5 -1.64 % ±8.41% ±11.19% ±14.58% 23:20:26 http/client-request-body.js method='write' len=32 type='asc' dur=5 3.12 % ±8.32% ±11.07% ±14.42% 23:20:26 http/client-request-body.js method='write' len=32 type='buf' dur=5 3.76 % ±9.14% ±12.16% ±15.83% 23:20:26 http/client-request-body.js method='write' len=32 type='utf' dur=5 3.15 % ±9.44% ±12.56% ±16.34% 23:20:26 http/cluster.js duration=5 c=500 len=102400 type='buffer' benchmarker='wrk' 0.60 % ±3.13% ±4.17% ±5.43% 23:20:26 http/cluster.js duration=5 c=500 len=102400 type='bytes' benchmarker='wrk' 2.21 % ±3.22% ±4.30% ±5.61% 23:20:26 http/cluster.js duration=5 c=500 len=1024 type='buffer' benchmarker='wrk' 0.17 % ±3.23% ±4.30% ±5.59% 23:20:26 http/cluster.js duration=5 c=500 len=1024 type='bytes' benchmarker='wrk' 0.35 % ±3.28% ±4.36% ±5.68% 23:20:26 http/cluster.js duration=5 c=500 len=4 type='buffer' benchmarker='wrk' -0.60 % ±3.42% ±4.55% ±5.92% 23:20:26 http/cluster.js duration=5 c=500 len=4 type='bytes' benchmarker='wrk' 1.82 % ±2.31% ±3.08% ±4.01% 23:20:26 http/cluster.js duration=5 c=50 len=102400 type='buffer' benchmarker='wrk' * 4.94 % ±3.74% ±4.98% ±6.48% 23:20:26 http/cluster.js duration=5 c=50 len=102400 type='bytes' benchmarker='wrk' -2.31 % ±3.62% ±4.81% ±6.27% 23:20:26 http/cluster.js duration=5 c=50 len=1024 type='buffer' benchmarker='wrk' -3.97 % ±5.67% ±7.55% ±9.82% 23:20:26 http/cluster.js duration=5 c=50 len=1024 type='bytes' benchmarker='wrk' 1.26 % ±6.17% ±8.21% ±10.70% 23:20:26 http/cluster.js duration=5 c=50 len=4 type='buffer' benchmarker='wrk' -3.58 % ±5.97% ±7.94% ±10.33% 23:20:26 http/cluster.js duration=5 c=50 len=4 type='bytes' benchmarker='wrk' -0.53 % ±5.66% ±7.53% ±9.80% 23:20:26 http/create-clientrequest.js e=1 arg='options' url='idn' -0.25 % ±8.27% ±11.00% ±14.32% 23:20:26 http/create-clientrequest.js e=1 arg='options' url='long' -5.66 % ±7.02% ±9.33% ±12.15% 23:20:26 http/create-clientrequest.js e=1 arg='options' url='wpt' -2.20 % ±7.11% ±9.46% ±12.31% 23:20:26 http/create-clientrequest.js e=1 arg='string' url='idn' 4.12 % ±6.83% ±9.11% ±11.89% 23:20:26 http/create-clientrequest.js e=1 arg='string' url='long' -3.20 % ±9.09% ±12.09% ±15.75% 23:20:26 http/create-clientrequest.js e=1 arg='string' url='wpt' -0.84 % ±5.77% ±7.68% ±9.99% 23:20:26 http/create-clientrequest.js e=1 arg='URL' url='idn' 1.91 % ±6.31% ±8.40% ±10.96% 23:20:26 http/create-clientrequest.js e=1 arg='URL' url='long' 0.03 % ±7.48% ±9.95% ±12.96% 23:20:26 http/create-clientrequest.js e=1 arg='URL' url='wpt' 1.19 % ±6.10% ±8.11% ±10.56% 23:20:26 http/end-vs-write-end.js duration=5 method='end' c=100 len=1048576 type='asc' benchmarker='wrk' -0.70 % ±2.56% ±3.41% ±4.45% 23:20:26 http/end-vs-write-end.js duration=5 method='end' c=100 len=1048576 type='buf' benchmarker='wrk' -2.56 % ±8.07% ±10.73% ±13.97% 23:20:26 http/end-vs-write-end.js duration=5 method='end' c=100 len=1048576 type='utf' benchmarker='wrk' 0.14 % ±1.98% ±2.64% ±3.44% 23:20:26 http/end-vs-write-end.js duration=5 method='end' c=100 len=131072 type='asc' benchmarker='wrk' -0.81 % ±2.10% ±2.80% ±3.66% 23:20:26 http/end-vs-write-end.js duration=5 method='end' c=100 len=131072 type='buf' benchmarker='wrk' -0.03 % ±2.37% ±3.15% ±4.10% 23:20:26 http/end-vs-write-end.js duration=5 method='end' c=100 len=131072 type='utf' benchmarker='wrk' 0.45 % ±1.49% ±1.99% ±2.59% 23:20:26 http/end-vs-write-end.js duration=5 method='end' c=100 len=262144 type='asc' benchmarker='wrk' -0.52 % ±1.53% ±2.04% ±2.66% 23:20:26 http/end-vs-write-end.js duration=5 method='end' c=100 len=262144 type='buf' benchmarker='wrk' * 5.79 % ±4.40% ±5.86% ±7.64% 23:20:26 http/end-vs-write-end.js duration=5 method='end' c=100 len=262144 type='utf' benchmarker='wrk' -0.01 % ±2.32% ±3.08% ±4.01% 23:20:26 http/end-vs-write-end.js duration=5 method='end' c=100 len=65536 type='asc' benchmarker='wrk' 0.23 % ±2.33% ±3.10% ±4.04% 23:20:26 http/end-vs-write-end.js duration=5 method='end' c=100 len=65536 type='buf' benchmarker='wrk' 1.37 % ±2.84% ±3.78% ±4.92% 23:20:26 http/end-vs-write-end.js duration=5 method='end' c=100 len=65536 type='utf' benchmarker='wrk' * -1.82 % ±1.80% ±2.40% ±3.12% 23:20:26 http/end-vs-write-end.js duration=5 method='write' c=100 len=1048576 type='asc' benchmarker='wrk' 0.10 % ±5.13% ±6.83% ±8.89% 23:20:26 http/end-vs-write-end.js duration=5 method='write' c=100 len=1048576 type='buf' benchmarker='wrk' 2.24 % ±7.30% ±9.72% ±12.66% 23:20:26 http/end-vs-write-end.js duration=5 method='write' c=100 len=1048576 type='utf' benchmarker='wrk' -0.21 % ±4.83% ±6.43% ±8.37% 23:20:26 http/end-vs-write-end.js duration=5 method='write' c=100 len=131072 type='asc' benchmarker='wrk' -0.11 % ±3.13% ±4.16% ±5.41% 23:20:26 http/end-vs-write-end.js duration=5 method='write' c=100 len=131072 type='buf' benchmarker='wrk' 0.90 % ±2.47% ±3.29% ±4.28% 23:20:26 http/end-vs-write-end.js duration=5 method='write' c=100 len=131072 type='utf' benchmarker='wrk' -1.88 % ±3.44% ±4.57% ±5.96% 23:20:26 http/end-vs-write-end.js duration=5 method='write' c=100 len=262144 type='asc' benchmarker='wrk' 2.08 % ±4.64% ±6.17% ±8.04% 23:20:26 http/end-vs-write-end.js duration=5 method='write' c=100 len=262144 type='buf' benchmarker='wrk' -3.69 % ±4.93% ±6.56% ±8.54% 23:20:26 http/end-vs-write-end.js duration=5 method='write' c=100 len=262144 type='utf' benchmarker='wrk' 2.09 % ±4.51% ±6.01% ±7.82% 23:20:26 http/end-vs-write-end.js duration=5 method='write' c=100 len=65536 type='asc' benchmarker='wrk' -0.63 % ±1.89% ±2.51% ±3.27% 23:20:26 http/end-vs-write-end.js duration=5 method='write' c=100 len=65536 type='buf' benchmarker='wrk' * 2.42 % ±1.91% ±2.54% ±3.30% 23:20:26 http/end-vs-write-end.js duration=5 method='write' c=100 len=65536 type='utf' benchmarker='wrk' -0.89 % ±2.05% ±2.72% ±3.55% 23:20:26 http/headers.js duration=5 len=100 n=1000 benchmarker='wrk' 2.03 % ±2.67% ±3.55% ±4.63% 23:20:26 http/headers.js duration=5 len=100 n=10 benchmarker='wrk' 4.50 % ±5.24% ±6.98% ±9.09% 23:20:26 http/headers.js duration=5 len=1 n=1000 benchmarker='wrk' 1.56 % ±2.33% ±3.11% ±4.04% 23:20:26 http/headers.js duration=5 len=1 n=10 benchmarker='wrk' 1.22 % ±5.07% ±6.75% ±8.81% 23:20:26 ```

src/node_http_parser.cc

addaleax · 2020-03-29T21:00:33Z

src/node_http_parser.cc

+      auto parsing_time = (now - header_parsing_start_time_) / 1e6;
+
+      if (parsing_time > headers_timeout_) {
+        Local<Value> cb =


@OrKoN I don’t think so. But if it only fires in the timeout case, that should be okay.

addaleax · 2020-03-29T21:01:25Z

src/node_http_parser.cc

+        if (!cb->IsFunction())
+          return;
+
+        MakeCallback(cb.As<Function>(), 0, nullptr);


@OrKoN Since you’d be return;ing here either way, whether the call threw an exception or not, I don’t think so. You can wrap the call in USE() to make it explicit that you’re discarding the return value because of that.

mcollina

lgtm

nodejs-github-bot · 2020-03-29T22:04:56Z

CI: https://ci.nodejs.org/job/node-test-pull-request/30220/

Co-Authored-By: Anna Henningsen <github@addaleax.net>

nodejs-github-bot · 2020-03-30T15:01:49Z

CI: https://ci.nodejs.org/job/node-test-pull-request/30257/

For keep-alive connections, the headersTimeout may fire during subsequent request because the measurement was reset after a request and not before a request. PR-URL: #32329 Fixes: #27363 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

mcollina · 2020-04-02T12:51:25Z

Landed in b149eef

For keep-alive connections, the headersTimeout may fire during subsequent request because the measurement was reset after a request and not before a request. PR-URL: #32329 Fixes: #27363 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

mcollina · 2020-04-10T07:27:27Z

@BethGriggs this should ideally be backported down to 10 in the next patch release. When is that scheduled? I assume this is going to need backport PRs, should we open them already?

BethGriggs · 2020-04-10T20:51:14Z

The next v12.x is due out 28th April and the next v10.x May 5th. Assuming that this commit goes out with v13.x current (scheduled next Tuesday), then this change should be able to make it into the upcoming releases. Opening backport PRs anytime after it has gone out in current (next Tuesday) makes sense to me.

Flarna · 2020-06-30T11:38:11Z

I created a backport PR: #34131

richardlau · 2020-07-02T13:41:55Z

This doesn't land cleanly on v10.x-staging, presumably for the same reason it does not land on 12.x.

Flarna · 2020-07-02T13:44:07Z

@richardlau Should I create one more backport PR for 10.x? My assumption was that it should be in 12.x for a while first.

richardlau · 2020-07-02T13:51:12Z

@Flarna I've been going around the lts-watch-v10.x labelled PRs to see what's landable for the upcoming 10.x release I'm preparing. Your assumption is correct that usually we'd want things to flow back in an ordered manner (so 12.x before 10.x). I haven't checked to see if the 12.x PR would land cleanly on 10.x. If it does we can add a lts-watch-v10.x label to the 12.x backport PR, otherwise yes please open a backport PR for 10.x so it can be ready for a future 10.x release (there's no regular schedule for non-security 10.x releases as it is now in maintenance -- the release team will review periodically to see if there is enough content to warrant a release).

Flarna · 2020-07-02T14:52:46Z

I did a fast check and it seems that the commit for 12.x doesn't land on 10.x so seems a dedicated backport PR will be needed. I may create one once the backport to 12.x is finished.

For keep-alive connections, the headersTimeout may fire during subsequent request because the measurement was reset after a request and not before a request. Backport-PR-URL: #34131 PR-URL: #32329 Fixes: #27363 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

andrewnester · 2020-10-20T16:49:22Z

@OrKoN Could this change cause the following issue with uploading?
#35661

PR-URL: nodejs#36630 Refs: nodejs#32329 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>

PR-URL: #36630 Refs: #32329 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>

PR-URL: nodejs#36630 Refs: nodejs#32329 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>

This reverts commit 9156f43. PR-URL: #36890 Refs: #32329 Reviewed-By: Danielle Adams <adamzdanielle@gmail.com>

PR-URL: #36630 Backport-PR-URL: #36890 Refs: #32329 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>

PR-URL: #36630 Refs: #32329 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>

ArnoldZokas · 2021-06-10T17:20:07Z

has this been backported to v14?

…46052) Resolves #39689, partially resolves #28642 (see notes below) Inspired by #44627 In #28642 it was also asked to expose `server.headersTimeout`, but it is probably not needed for most use cases and not implemented even in `next start`. It was needed to change this option before nodejs/node#27363. There also exists a rare bug that is described here nodejs/node#32329 (comment). To fix this exposing `server.headersTimeout` might be required both in `server.js` and in `next start`. Co-authored-by: JJ Kasper <jj@jjsweb.site>

For keep-alive connections, the headersTimeout may fire during subsequent request because the measurement was reset after a request and not before a request. PR-URL: nodejs/node#32329 Fixes: nodejs/node#27363 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

OrKoN added 2 commits March 17, 2020 18:10

http: fix incorrect headersTimeout measurement

ed7b316

For keep-alive connections, the headersTimeout may fire during subsequent request because the measurement was reset after a request and not before a request. Fixes: nodejs#27363

Clean up

f162c42

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 17, 2020

OrKoN commented Mar 17, 2020

View reviewed changes

OrKoN mentioned this pull request Mar 19, 2020

http: fix incorrect headersTimeout measurement #30184

Closed

3 tasks

OrKoN changed the title ~~http: fix incorrect headersTimeout measurement~~ http: fix incorrect headersTimeout measurement (alt) Mar 19, 2020

Update async_hooks test

2d91c30

addaleax approved these changes Mar 29, 2020

View reviewed changes

addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. review wanted PRs that need reviews. labels Mar 29, 2020

mcollina approved these changes Mar 29, 2020

View reviewed changes

Update src/node_http_parser.cc

9536c1b

Co-Authored-By: Anna Henningsen <github@addaleax.net>

mcollina closed this Apr 2, 2020

mcollina added lts-watch-v10.x labels Apr 9, 2020

richardlau added the backport-requested-v10.x label Jul 2, 2020

Flarna added the backport-open-v12.x label Jul 10, 2020

Flarna added backported-to-v12.x and removed backport-open-v12.x labels Aug 18, 2020

codebytere mentioned this pull request Sep 28, 2020

v12.19.0 proposal #35401

Merged

andrewnester mentioned this pull request Oct 20, 2020

HTTP: Connection resets after 60 seconds in node.js upload application #35661

Closed

MylesBorins removed the lts-watch-v12.x label Nov 16, 2020

Lxxyx mentioned this pull request Dec 25, 2020

lib: remove dead code from internal/http.js #36630

Merged

2 tasks

aduh95 mentioned this pull request Jan 12, 2021

[Backport v15.x] http: remove dead code from internal/http.js #36890

Closed

danielleadams pushed a commit that referenced this pull request Jan 13, 2021

Revert "http: remove dead code from internal/http.js"

7e3ad1b

This reverts commit 9156f43. PR-URL: #36890 Refs: #32329 Reviewed-By: Danielle Adams <adamzdanielle@gmail.com>

AdamKatzDev mentioned this pull request Feb 17, 2023

feat(standalone): allow configuring KEEP_ALIVE_TIMEOUT via env var vercel/next.js#46052

Merged

codyebberson mentioned this pull request Jun 19, 2023

Fixes #2253 - increase server keep alive timeout medplum/medplum#2256

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: fix incorrect headersTimeout measurement (alt) #32329

http: fix incorrect headersTimeout measurement (alt) #32329

OrKoN commented Mar 17, 2020 •

edited

OrKoN Mar 17, 2020

OrKoN Mar 17, 2020

OrKoN Mar 17, 2020

addaleax Mar 29, 2020

OrKoN Mar 17, 2020

addaleax Mar 29, 2020

OrKoN commented Mar 17, 2020

OrKoN commented Mar 19, 2020

mcollina commented Mar 19, 2020

mcollina commented Mar 19, 2020

addaleax Mar 29, 2020

addaleax Mar 29, 2020

mcollina left a comment

nodejs-github-bot commented Mar 29, 2020

nodejs-github-bot commented Mar 30, 2020

mcollina commented Apr 2, 2020

mcollina commented Apr 10, 2020

BethGriggs commented Apr 10, 2020

Flarna commented Jun 30, 2020

richardlau commented Jul 2, 2020

Flarna commented Jul 2, 2020 •

edited

richardlau commented Jul 2, 2020

Flarna commented Jul 2, 2020

andrewnester commented Oct 20, 2020

ArnoldZokas commented Jun 10, 2021

http: fix incorrect headersTimeout measurement (alt) #32329

http: fix incorrect headersTimeout measurement (alt) #32329

Conversation

OrKoN commented Mar 17, 2020 • edited

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OrKoN commented Mar 17, 2020

OrKoN commented Mar 19, 2020

mcollina commented Mar 19, 2020

mcollina commented Mar 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Mar 29, 2020

nodejs-github-bot commented Mar 30, 2020

mcollina commented Apr 2, 2020

mcollina commented Apr 10, 2020

BethGriggs commented Apr 10, 2020

Flarna commented Jun 30, 2020

richardlau commented Jul 2, 2020

Flarna commented Jul 2, 2020 • edited

richardlau commented Jul 2, 2020

Flarna commented Jul 2, 2020

andrewnester commented Oct 20, 2020

ArnoldZokas commented Jun 10, 2021

OrKoN commented Mar 17, 2020 •

edited

Flarna commented Jul 2, 2020 •

edited