Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http: fix incorrect headersTimeout measurement (alt) #32329

Closed
wants to merge 4 commits into from

Conversation

OrKoN
Copy link
Contributor

@OrKoN OrKoN commented Mar 17, 2020

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
@nodejs-github-bot 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
@@ -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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return;
}
function onParserTimeout(server, socket) {
const serverTimeout = server.emit('timeout', socket);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

time tracking is moved to c++ code

if (!cb->IsFunction())
return;

MakeCallback(cb.As<Function>(), 0, nullptr);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

auto parsing_time = (now - header_parsing_start_time_) / 1e6;

if (parsing_time > headers_timeout_) {
Local<Value> cb =
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@OrKoN
Copy link
Contributor Author

OrKoN commented Mar 17, 2020

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 OrKoN changed the title http: fix incorrect headersTimeout measurement http: fix incorrect headersTimeout measurement (alt) Mar 19, 2020
@OrKoN
Copy link
Contributor Author

OrKoN commented Mar 19, 2020

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
Copy link
Member

@mcollina
Copy link
Member

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 Outdated Show resolved Hide resolved
auto parsing_time = (now - header_parsing_start_time_) / 1e6;

if (parsing_time > headers_timeout_) {
Local<Value> cb =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (!cb->IsFunction())
return;

MakeCallback(cb.As<Function>(), 0, nullptr);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

@addaleax 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
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@nodejs-github-bot
Copy link
Collaborator

Co-Authored-By: Anna Henningsen <github@addaleax.net>
@nodejs-github-bot
Copy link
Collaborator

mcollina pushed a commit that referenced this pull request Apr 2, 2020
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
Copy link
Member

mcollina commented Apr 2, 2020

Landed in b149eef

@mcollina mcollina closed this Apr 2, 2020
BethGriggs pushed a commit that referenced this pull request Apr 7, 2020
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
Copy link
Member

@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
Copy link
Member

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
Copy link
Member

Flarna commented Jun 30, 2020

I created a backport PR: #34131

@richardlau
Copy link
Member

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

@Flarna
Copy link
Member

Flarna commented Jul 2, 2020

@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
Copy link
Member

@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
Copy link
Member

Flarna commented Jul 2, 2020

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.

MylesBorins pushed a commit that referenced this pull request Aug 18, 2020
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>
@codebytere codebytere mentioned this pull request Sep 28, 2020
@andrewnester
Copy link

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

Trott pushed a commit to Lxxyx/node that referenced this pull request Dec 27, 2020
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>
danielleadams pushed a commit that referenced this pull request Jan 12, 2021
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>
aduh95 pushed a commit to aduh95/node that referenced this pull request Jan 13, 2021
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>
danielleadams pushed a commit that referenced this pull request Jan 13, 2021
This reverts commit 9156f43.

PR-URL: #36890
Refs: #32329
Reviewed-By: Danielle Adams <adamzdanielle@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 13, 2021
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>
targos pushed a commit that referenced this pull request May 1, 2021
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
Copy link
Contributor

has this been backported to v14?

ijjk added a commit to vercel/next.js that referenced this pull request Mar 17, 2023
…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>
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this pull request May 5, 2024
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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. review wanted PRs that need reviews.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression issue with keep alive connections