http: fix http server connection list when close

[theanarkh]

Refs: Failed CI
Refs: Failed CI
Refs: #43638 (comment)

When the socket close, the related parser should be removed out of the server connection list.

When the socket close, the related parser will be cleared(_http_server.js:socketOnClose -> freeParser), the related code of freeParser is as follows.

cleanParser(parser);
if (parsers.free(parser) === false) {
    setImmediate(closeParserInstance, parser);
} else {
    parser.free();
}

cleanParser(parser) make the socket.parser is null, and if parsers.free(parser) is true anything is ok because it will remove the parser out of server.connectionList. But if it is false it will trigger bugs.

1. If the user listen to close event of socket and call server.close in the callback of close event, the server.close will call closeIdleConnections to close all idle sockets, the code is as follows.

Server.prototype.closeIdleConnections = function() {
    const connections = this[kConnections].idle();
    for (let i = 0, l = connections.length; i < l; i++) {
        if (connections[i].socket._httpMessage && !connections[i].socket._httpMessage.finished) {
          continue;
        }
    }
};

connections[i] is a parser object, and parser.socket is null which make the bug(Cannot read properties of null (reading '_httpMessage')).

2. If user call the code as follows in callback of close event

setImmediate(() => {
    server.close();
});

it will make the process crashed because when closeParserInstance in setImmediate(closeParserInstance, parser); is called, it will delete the C++ parser object. Then Node.js will call the second callback(() => { server.close(); }) of setImmediate, server.close() will call this[kConnections].idle() which will access C++ layer to get the parser list, but the parser is delete so make the process crashed. The codedump is as follows.

    frame #0: 0x0000000101cf0498 node`v8::internal::JSObject::AddDataElement(v8::internal::Handle<v8::internal::JSObject>, unsigned int, v8::internal::Handle<v8::internal::Object>, v8::internal::PropertyAttributes) [inlined] v8::internal::Handle<v8::internal::Object>::operator*(this=<unavailable>) const at handles.h:141:37 [opt]
    frame #1: 0x0000000101cf0498 node`v8::internal::JSObject::AddDataElement(v8::internal::Handle<v8::internal::JSObject>, unsigned int, v8::internal::Handle<v8::internal::Object>, v8::internal::PropertyAttributes) [inlined] v8::internal::Handle<v8::internal::Object>::operator->(this=<unavailable>) const at handles.h:135 [opt]
    frame #2: 0x0000000101cf0498 node`v8::internal::JSObject::AddDataElement(object=Handle<v8::internal::JSObject> @ 0x00007ffeee585a28, index=<unavailable>, value=<unavailable>, attributes=NONE) at js-objects.cc:5194 [opt]
    frame #3: 0x0000000101d535f2 node`v8::internal::Object::AddDataProperty(it=0x00007ffeee585ae0, value=<unavailable>, attributes=<unavailable>, should_throw=(has_value_ = true, value_ = kDontThrow), store_origin=<unavailable>, semantics=<unavailable>) at objects.cc:2914:5 [opt]
    frame #4: 0x00000001018c5efc node`v8::Object::Set(v8::Local<v8::Context>, unsigned int, v8::Local<v8::Value>) [inlined] v8::internal::Object::SetElement(isolate=<unavailable>, object=Handle<v8::internal::Object> @ r15, index=<unavailable>, value=Handle<v8::internal::Object> @ r14, should_throw=kDontThrow) at objects-inl.h:657:3 [opt]
    frame #5: 0x00000001018c5ec1 node`v8::Object::Set(this=0x00007fb667814658, context=<unavailable>, index=<unavailable>, value=(val_ = 0x0000000000000000)) at api.cc:4282 [opt]
    frame #6: 0x000000010176499c node`node::(anonymous namespace)::ConnectionsList::Idle(args=0x00007ffeee585c50) at node_http_parser.cc:1065:17 [opt]
    frame #7: 0x000000010191c228 node`v8::internal::FunctionCallbackArguments::Call(this=0x00007ffeee585ca8, handler=<unavailable>) at api-arguments-inl.h:147:3 [opt]
    frame #8: 0x000000010191bd42 node`v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(isolate=0x00007fb66752b000, function=<unavailable>, new_target=<unavailable>, fun_data=Handle<v8::internal::FunctionTemplateInfo> @ r13, receiver=<unavailable>, args=BuiltinArguments @ 0x00007ffeee585d50) at builtins-api.cc:112:36 [opt]
    frame #9: 0x000000010191b38f node`v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) at builtins-api.cc:142:5 [opt]
    frame #10: 0x000000010191b2aa node`v8::internal::Builtin_HandleApiCall(args_length=<unavailable>, args_object=0x00007ffeee585e00, isolate=0x00007fb66752b000) at builtins-api.cc:130 [opt]
    frame #11: 0x000000010225d479 node`Builtins_CEntry_Return1_DontSaveFPRegs_ArgvOnStack_BuiltinExit + 57
    frame #12: 0x00000001021e13d0 node`Builtins_InterpreterEntryTrampoline + 208
    frame #13: 0x00000001021e13d0 node`Builtins_InterpreterEntryTrampoline + 208
    frame #14: 0x00000001021e13d0 node`Builtins_InterpreterEntryTrampoline + 208
    frame #15: 0x00000001021e13d0 node`Builtins_InterpreterEntryTrampoline + 208
    frame #16: 0x00000001021df9dc node`Builtins_JSEntryTrampoline + 92
    frame #17: 0x00000001021df703 node`Builtins_JSEntry + 131
    frame #18: 0x00000001019e68bc node`v8::internal::(anonymous namespace)::Invoke(v8::internal::Isolate*, v8::internal::(anonymous namespace)::InvokeParams const&) [inlined] v8::internal::GeneratedCode<unsigned long, unsigned long, unsigned long, unsigned long, unsigned long, long, unsigned long**>::Call(this=<unavailable>, args=<unavailable>, args=<unavailable>, args=<unavailable>, args=<unavailable>, args=<unavailable>, args=<unavailable>) at simulator.h:156:12 [opt]
    frame #19: 0x00000001019e68b9 node`v8::internal::(anonymous namespace)::Invoke(isolate=0x00007fb66752b000, params=0x00007ffeee586190)::InvokeParams const&) at execution.cc:425 [opt]
    frame #20: 0x00000001019e5ca5 node`v8::internal::Execution::Call(isolate=0x00007fb66752b000, callable=<unavailable>, receiver=<unavailable>, argc=0, argv=0x0000000000000000) at execution.cc:523:10 [opt]
    frame #21: 0x00000001018cc5f9 node`v8::Function::Call(this=0x00007fb66783a520, context=<unavailable>, recv=<unavailable>, argc=<unavailable>, argv=0x0000000000000000) at api.cc:5255:7 [opt]
    frame #22: 0x000000010167af2f node`node::InternalMakeCallback(env=0x00007fb66814e600, resource=(val_ = 0x00007fb66783a600), recv=<unavailable>, callback=<unavailable>, argc=0, argv=0x0000000000000000, asyncContext=(async_id = 0, trigger_async_id = 0)) at callback.cc:211:21 [opt]
    frame #23: 0x000000010167b33c node`node::MakeCallback(isolate=0x00007fb66752b000, recv=(val_ = 0x00007fb66783a600), callback=(val_ = 0x00007fb66783a520), argc=0, argv=0x0000000000000000, asyncContext=(async_id = 0, trigger_async_id = 0)) at callback.cc:282:7 [opt]
    frame #24: 0x00000001016e8645 node`node::Environment::CheckImmediate(handle=<unavailable>) at env.cc:1331:5 [opt]
    frame #25: 0x00000001021c87c4 node`uv__run_check(loop=0x00000001053cb630) at loop-watcher.c:67:1 [opt]
    frame #26: 0x00000001021c1d31 node`uv_run(loop=0x00000001053cb630, mode=UV_RUN_DEFAULT) at core.c:398:5 [opt]
    frame #27: 0x000000010167b743 node`node::SpinEventLoop(env=0x00007fb66814e600) at embed_helpers.cc:37:7 [opt]
    frame #28: 0x00000001017879e1 node`node::NodeMainInstance::Run(this=<unavailable>, exit_code=0x00007ffeee5866fc, env=0x00007fb66814e600) at node_main_instance.cc:140:18 [opt]
    frame #29: 0x0000000101787653 node`node::NodeMainInstance::Run(this=<unavailable>) at node_main_instance.cc:132:3 [opt]
    frame #30: 0x00000001017170c6 node`node::Start(argc=<unavailable>, argv=<unavailable>) at node.cc:1199:38 [opt]
    frame #31: 0x00007fff205fe621 libdyld.dylib`start + 1
    frame #32: 0x00007fff205fe621 libdyld.dylib`start + 1

cc @ShogunPanda

• 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

[nodejs-github-bot]

Review requested:

• @nodejs/http
• @nodejs/net

[mcollina]

lgtm

[ShogunPanda]

LGTM!

Thanks for taking care of this!

[nodejs-github-bot]

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

[mcollina]

Let's fast-track this.

cc @tniessen

[github-actions]

Fast-track has been requested by @mcollina. Please 👍 to approve.

[github-actions]

Fast-track has been requested by @F3n67u. Please 👍 to approve.

[F3n67u]

Sorry that I remove fast-track label wrongly. add it again.

[F3n67u]

node-stress-single-test comparison for sequential/test-gc-http-client-timeout

This pr (No Failure)

https://ci.nodejs.org/view/Stress/job/node-stress-single-test/359/

Main Branch (Failure)

https://ci.nodejs.org/job/node-stress-single-test/nodes=ubuntu2004-arm64/357/

Don't know why the error is crash but not Cannot read properties of null

Error Message
crashed (-11)
Stacktrace
done/collected/total: 157/0/804
done/collected/total: 322/157/804
done/collected/total: 479/322/804
done/collected/total: 801/413/804
done/collected/total: 801/479/804
done/collected/total: 801/801/804
done/collected/total: 804/801/804
done/collected/total: 804/801/804
done/collected/total: 804/804/804

[F3n67u]

Trigger another stress test for sequential/test-gc-http-client & sequential/test-gc-http-client-onerror to verfiy if those flaky test is fixed: https://ci.nodejs.org/view/Stress/job/node-stress-single-test/360/

node-stress-single-test comparison for test-gc-http-client & test-gc-http-client-onerror

This pr (No Failure)

https://ci.nodejs.org/view/Stress/job/node-stress-single-test/360/

Main Branch (Failure)

https://ci.nodejs.org/view/Stress/job/node-stress-single-test/361/

[theanarkh]

node-stress-single-test comparison for sequential/test-gc-http-client-timeout

This pr (No Failure)

https://ci.nodejs.org/view/Stress/job/node-stress-single-test/359/

Main Branch (Failure)

https://ci.nodejs.org/job/node-stress-single-test/nodes=ubuntu2004-arm64/357/

Don't know why the error is crash but not Cannot read properties of null

Error Message
crashed (-11)
Stacktrace
done/collected/total: 157/0/804
done/collected/total: 322/157/804
done/collected/total: 479/322/804
done/collected/total: 801/413/804
done/collected/total: 801/479/804
done/collected/total: 801/801/804
done/collected/total: 804/801/804
done/collected/total: 804/801/804
done/collected/total: 804/804/804

Thanks for testing this. Cannot read properties of null and crash is two bugs this pr try to fix. If there are too many requests it will often trigger crash.

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/43949
✔  Done loading data for nodejs/node/pull/43949
----------------------------------- PR info ------------------------------------
Title      http: fix http server connection list when close (#43949)
Author     theanarkh  (@theanarkh)
Branch     theanarkh:fix_server_connection_list -> nodejs:main
Labels     http, c++, http_parser, fast-track, author ready
Commits    1
 - http: fix http server connection list when close
Committers 1
 - theanarkh 
PR-URL: https://github.com/nodejs/node/pull/43949
Reviewed-By: Matteo Collina 
Reviewed-By: Paolo Insogna 
Reviewed-By: Feng Yu 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/43949
Reviewed-By: Matteo Collina 
Reviewed-By: Paolo Insogna 
Reviewed-By: Feng Yu 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Fri, 22 Jul 2022 18:59:11 GMT
   ✔  Approvals: 3
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/43949#pullrequestreview-1048331522
   ✔  - Paolo Insogna (@ShogunPanda): https://github.com/nodejs/node/pull/43949#pullrequestreview-1048515689
   ✔  - Feng Yu (@F3n67u): https://github.com/nodejs/node/pull/43949#pullrequestreview-1048600770
   ℹ  This PR is being fast-tracked
   ✖  Last GitHub CI failed
   ℹ  Last Full PR CI on 2022-07-24T09:13:51Z: https://ci.nodejs.org/job/node-test-pull-request/45629/
- Querying data for job/node-test-pull-request/45629/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/2727070300

[nodejs-github-bot]

Landed in da3949c

[targos]

Should this be backported? parser->connectionsList_ doesn't exist in v16.x

[theanarkh]

If parser->connectionsList_ doesn't exist in v16.x, i think we can not backport this to v16.x.