GeezerVison

Review requested:

• @nodejs/crypto
• @nodejs/http
• @nodejs/net

I'm not confident approving this without a test

I looked and couldn't find any tests for this specific server.close behavior. Obviously, this doesn't mean that they don't exist - If you know where they exist, or can give me some pointers on how to test this for http and https, I'd be happy to port or write them. If you have any other fears of breakage (e.g. because close is now on https.Server instead of tls.Server) I'd be happy to add any test that is missing as well.

The original intention that was approved here was to add it to https as well, but it was just an oversight that https.Server.close doesn't actually call http.Server.close.

Shouldn't it be possible to write a minimal test based on the code in the original issue?

Shouldn't it be possible to write a minimal test based on the code in the original issue?

I assumed that the question was something more functional than that. If it's just adding a test to check that the timer was cleared, it's not very difficult to check (The test would know some internals, as the timer hides behind a symbol so we'd need to expose it, but IMO it's not that bad).

I second @marco-ippolito. Code changes look fine but we need tests, thanks :)

added tests as requested, note that I also changed an existing test as the call to closeIdleConnections is redundant because close already calls closeIdleConnections.

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

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

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

This PR broke citgm on v18.18.0-proposal. It's going to require manual backport and citgm validation before landing on v18.x-staging again.

How did it broke CITGM?

oh sorry I forgot to add some more details, it broke [email protected] on almost every platform (which might very much be of your interest @mcollina), here's a summary of the errors:

# Subtest: test/close-pipelining.test.js
     # Subtest: Should return 503 while closing - pipelining
         ok 1 - should be equivalent
         1..1
     ok 1 - Should return 503 while closing - pipelining # time=117.997ms
     
     # Subtest: Should not return 503 while closing - pipelining - return503OnClosing: false, skip Node >= v19.x
         not ok 1 - other side closed
           ---
           stack: |
             Socket.onSocketEnd (node_modules/undici/lib/client.js:1116:22)
           at:
             line: 1116
             column: 22
             file: node_modules/undici/lib/client.js
             function: Socket.onSocketEnd
           type: SocketError
           code: UND_ERR_SOCKET
           socket:
             localAddress: ::1
             localPort: 53232
             remoteAddress: null
             remotePort: null
             remoteFamily: null
             timeout: null
             bytesWritten: 195
             bytesRead: 376
           tapCaught: returnedPromiseRejection
           test: "Should not return 503 while closing - pipelining - return503OnClosing:
             false, skip Node >= v19.x"
           source: >2
           
               util.destroy(this, new SocketError('other side closed', util.getSocketInfo(this)))
             ---------------------^
           
             }
           ...
         
         1..1
         # failed 1 test
     not ok 2 - Should not return 503 while closing - pipelining - return503OnClosing: false, skip Node >= v19.x # time=61.927ms

     # Subtest: Current opened connection should continue to work after closing and return "connection: close" header - return503OnClosing: false, skip Node >= v19.x
         ok 1 - should not error
         ok 2 - should match pattern provided
         ok 3 - should match pattern provided
         not ok 4 - test unfinished
           ---
           stack: |
             Object.<anonymous> (test/close.test.js:207:1)
           test: 'Current opened connection should continue to work after closing and
             return "connection: close" header - return503OnClosing: false, skip Node >=
             v19.x'
           at:
             line: 207
             column: 1
             file: test/close.test.js
             function: Object.<anonymous>
           source: >
             const isV19plus = semver.gte(process.version, '19.0.0')
           
             test('Current opened connection should continue to work after closing and return "connection: close" header - return503OnClosing: false, skip Node >= v19.x', { skip: isV19plus }, t => {
           
             ^
               const fastify = Fastify({
                 return503OnClosing: false,
           ...
         
         1..4
         # failed 1 of 4 tests
     not ok 9 - Current opened connection should continue to work after closing and return "connection: close" header - return503OnClosing: false, skip Node >= v19.x # time=71.912ms

And here is the link to the full CITGM run: https://ci.nodejs.org/job/citgm-smoker/3229/

it looks like it also broke [email protected] on Linux. I'm recreating the proposal without this commit and running CITGM again, you can see the results here: https://ci.nodejs.org/job/citgm-smoker/3237/

why have we changed the behavior in minor release? i.e Note that this also adds a call to closeIdleConnections() to https.server.close .

Is there any way to keep old behavior on v18. I completely understand and agree with adding a new method to handle it but modifying the existing one i.e server.close, it broke our app/tests.

In an environment where we own client and server, we usually keep client timeout less than server keepalive timeout and let the client know by connection:close header to handle future request. Scenario: There could be multiple stack in different language talking to each other.

related to #52330

CC: @mcollina @ShogunPanda Thoughts

why have we changed the behavior in minor release? i.e Note that this also adds a call to closeIdleConnections() to https.server.close .

Is there any way to keep old behavior on v18. I completely understand and agree with adding a new method to handle it but modifying the existing one i.e server.close, it broke our app/tests.

In an environment where we own client and server, we usually keep client timeout less than server keepalive timeout and let the client know by connection:close header to handle future request. Scenario: There could be multiple stack in different language talking to each other.

related to #52330

CC: @mcollina @ShogunPanda Thoughts

This wasn't a semver-minor, it was a bug fix for a Node 19 change that was added here that missed that the added behavior to HTTP did not automatically apply to HTTPS, which was marked at the time as a semver-major.

If I'm understanding correctly, the backport to 18 has a mistake #50194, it shouldn't have added server.closeIdleConnections();. It should have only extracted the clear timeout to a "shared" method, without adding more functionality. Obviously it was not intentional (as indeed, the behavior change was originally marked as semver major). A PR would be welcome.