http: keep HTTP/1.1 conns alive even if the Connection header is removed

[pimterry]

Previously persistence was completed disabled if you removed this header, which is not correct for modern HTTP, where the header is optional and all connections should persist by default regardless. See #46321 for more context.

This header is required to enable keep alive for HTTP/1.0 and older. This PR doesn't explicitly check the version, but the shouldKeepAlive variable that's used here is set by the HTTP parser, which does check this (among other conditions) and there's a test here to confirm that.

As noted in #46321, we should test this for ecosystem impact, since this incorrect default dates back to 2012 and it may be relied on elsewhere.

Fixes #46321

[nodejs-github-bot]

Review requested:

• @nodejs/http
• @nodejs/net

[ShogunPanda]

LGTM. Let's run CITGM first and then I'll approve.

[mcollina]

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3082/

[mcollina]

lgtm

[nodejs-github-bot]

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

[mcollina]

This breaks both Fastify and Undici.

Here is the fastify break:

ok 9 - Current opened connection should continue to work after closing and return "connection: close" header - return503OnClosing: false, skip Node >= v19.x # SKIP
     # Subtest: Current opened connection should NOT continue to work after closing and return "connection: close" header - return503OnClosing: false, skip Node < v19.x
         1..4
         ok 1 - should not error
         not ok 2 - should match pattern provided
           ---
           diff: |
             --- expected
             +++ actual
             @@ -1,1 +1,10 @@
             -/Connection:\s*keep-alive/i
             +String(
             +  HTTP/1.1 400 Bad Request\r
             +  Connection: close\r
             +  Date: Tue, 24 Jan 2023 14:13:48 GMT\r
             +  Transfer-Encoding: chunked\r
             +  \r
             +  0\r
             +  \r
             +  
             +)
           pattern: !re /Connection:\s*keep-alive/i
           at:
             line: 275
             column: 11
             file: test/close.test.js
             type: Socket
           stack: |
             Socket.<anonymous> (test/close.test.js:275:11)
           source: |2
                   client.once('data', data => {
                     t.match(data.toString(), /Connection:\s*keep-alive/i)
             ----------^
                     t.match(data.toString(), /200 OK/i)
           ...
         
         not ok 3 - should match pattern provided
           ---
           diff: |
             --- expected
             +++ actual
             @@ -1,1 +1,10 @@
             -/200 OK/i
             +String(
             +  HTTP/1.1 400 Bad Request\r
             +  Connection: close\r
             +  Date: Tue, 24 Jan 2023 14:13:48 GMT\r
             +  Transfer-Encoding: chunked\r
             +  \r
             +  0\r
             +  \r
             +  
             +)
           pattern: !re /200 OK/i
           at:
             line: 276
             column: 11
             file: test/close.test.js
             type: Socket
           stack: |
             Socket.<anonymous> (test/close.test.js:276:11)
           source: |2
                     t.match(data.toString(), /Connection:\s*keep-alive/i)
                     t.match(data.toString(), /200 OK/i)
             ----------^
           
                     client.write('GET / HTTP/1.1\r\n\r\n')
           ...
         
         ok 4 - close
         # failed 2 of 4 tests
     not ok 10 - Current opened connection should NOT continue to work after closing and return "connection: close" header - return503OnClosing: false, skip Node < v19.x # time=190.413ms
    

Undici:

 # Subtest: test/abort-controller.js
     1..0 # no tests found
 not ok 1 - test/abort-controller.js # time=149.983ms
   ---
   env: {}
   file: test/abort-controller.js
   timeout: 60000
   command: /home/iojs/build/workspace/citgm-smoker/smoker/bin/node
   args:
     - --expose-gc
     - --experimental-wasm-simd
     - test/abort-controller.js
   stdio:
     - 0
     - pipe
     - 2
   cwd: /home/iojs/tmp/citgm_tmp/5a4a14ca-e0e8-4e87-8f27-1efecbd6d768/undici
   exitCode: 9
   ...
 # Subtest: test/abort-event-emitter.js
     1..0 # no tests found
 not ok 2 - test/abort-event-emitter.js # time=187.344ms
   ---
   env: {}
   file: test/abort-event-emitter.js
   timeout: 60000
   command: /home/iojs/build/workspace/citgm-smoker/smoker/bin/node
   args:
     - --expose-gc
     - --experimental-wasm-simd
     - test/abort-event-emitter.js
   stdio:
     - 0
     - pipe
     - 2
   cwd: /home/iojs/tmp/citgm_tmp/5a4a14ca-e0e8-4e87-8f27-1efecbd6d768/undici
   exitCode: 9
   ...

[pimterry]

That Undici failure doesn't appear obviously connected to this change. It's complaining no tests found during some AbortController testing, without appearing to touch the Connection header anywhere. I'm not sure how CITGM works, but does that same test definitely pass reliably without this PR?

The Fastify one certainly seems related though, I'll dig into it.

[pimterry]

The Fastify issue appears unrelated. That test does fail for me locally, but fails in exactly the same way on the preceding main commit, without this PR.

I did some digging, the failure there comes from an unexpected 400 error that was added by this commit. You can see in the test that they're manually constructing a test HTTP request, without including a Host header, and so it's now being (correctly) rejected by Node after that change.

That's nothing to do with this change (and it's reasonable semver-major breakage imo - their test suite is sending an invalid HTTP request and it really should fail).

I checked on Undici too, just in case. Those tests pass locally for me using this code, and I can't see any relationship between them and these changes, so I think it's likely that that failure is some kind of intermittent or otherwise unrelated issue.

[mcollina]

sgtm, this can land

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 2a29df6

[ruyadorno]

Sorry for the noise in here but for a brief moment a commit from this PR landed (9c0de7d) in the v18.x-staging branch as it was part of a larger backport PR. I noticed (after landing the commit) the semver-major label and have then removed 9c0de7d from v18.x-staging.