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

https: fix connection checking interval not clearing on server close #48383

Merged
merged 2 commits into from
Jun 12, 2023

Conversation

Linkgoron
Copy link
Member

The connection checking interval should get cleared when https.Server.close is called similarly to how it gets cleared when http.Server.close is called. Note that this also adds a call to closeIdleConnections() to https.server.close.

Fixes #48373

The connection interval should close when httpsServer.close is called
similarly to how it gets cleared when httpServer.close is called.

fixes: nodejs#48373
@nodejs-github-bot
Copy link
Collaborator

Review requested:

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

@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. https Issues or PRs related to the https subsystem. needs-ci PRs that need a full CI run. labels Jun 7, 2023
Copy link
Member

@marco-ippolito marco-ippolito left a comment

Choose a reason for hiding this comment

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

I'm not confident approving this without a test

@Linkgoron
Copy link
Member Author

Linkgoron commented Jun 7, 2023

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.

@mscdex
Copy link
Contributor

mscdex commented Jun 7, 2023

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

@Linkgoron
Copy link
Member Author

Linkgoron commented Jun 7, 2023

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

@ShogunPanda
Copy link
Contributor

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

@Linkgoron
Copy link
Member Author

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

Copy link
Member

@MoLow MoLow left a comment

Choose a reason for hiding this comment

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

LGTM

@marco-ippolito marco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 8, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 8, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 8, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 8, 2023
@nodejs-github-bot
Copy link
Collaborator

const { createServer } = require('http');
const { kConnectionsCheckingInterval } = require('_http_server');

const server = createServer(function(req, res) {});
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const server = createServer(function(req, res) {});
const server = createServer();

cert: fixtures.readKey('agent1-cert.pem')
};

const server = createServer(options, function(req, res) {});
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const server = createServer(options, function(req, res) {});
const server = createServer(options);

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 awesome. I think I suffered from this bug as well.

@ShogunPanda ShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 10, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 10, 2023
@nodejs-github-bot
Copy link
Collaborator

@ShogunPanda ShogunPanda added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jun 10, 2023
RafaelGSS pushed a commit that referenced this pull request Jul 3, 2023
The connection interval should close when httpsServer.close is called
similarly to how it gets cleared when httpServer.close is called.

fixes: #48373
PR-URL: #48383
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Jul 3, 2023
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
The connection interval should close when httpsServer.close is called
similarly to how it gets cleared when httpServer.close is called.

fixes: nodejs#48373
PR-URL: nodejs#48383
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
The connection interval should close when httpsServer.close is called
similarly to how it gets cleared when httpServer.close is called.

fixes: nodejs#48373
PR-URL: nodejs#48383
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
ruyadorno pushed a commit that referenced this pull request Sep 7, 2023
The connection interval should close when httpsServer.close is called
similarly to how it gets cleared when httpServer.close is called.

fixes: #48373
PR-URL: #48383
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Sep 8, 2023
@ruyadorno
Copy link
Member

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.

@ruyadorno ruyadorno added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Sep 12, 2023
@mcollina
Copy link
Member

How did it broke CITGM?

@ruyadorno
Copy link
Member

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/

@ruyadorno
Copy link
Member

ruyadorno commented Sep 13, 2023

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/

sparist pushed a commit to Distributive-Network/node that referenced this pull request Sep 13, 2023
The connection interval should close when httpsServer.close is called
similarly to how it gets cleared when httpServer.close is called.

fixes: nodejs#48373
PR-URL: nodejs#48383
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
sparist pushed a commit to Distributive-Network/node that referenced this pull request Sep 13, 2023
The connection interval should close when httpsServer.close is called
similarly to how it gets cleared when httpServer.close is called.

fixes: nodejs#48373
PR-URL: nodejs#48383
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
H4ad pushed a commit to H4ad/node that referenced this pull request Sep 14, 2023
The connection interval should close when httpsServer.close is called
similarly to how it gets cleared when httpServer.close is called.

fixes: nodejs#48373
PR-URL: nodejs#48383
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@targos targos added backported-to-v18.x PRs backported to the v18.x-staging branch. and removed backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. labels Nov 27, 2023
targos pushed a commit that referenced this pull request Nov 27, 2023
The connection interval should close when httpsServer.close is called
similarly to how it gets cleared when httpServer.close is called.

Fixes: #48373
PR-URL: #48383
Backport-PR-URL: #50194
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@kumarrishav
Copy link
Contributor

kumarrishav commented Apr 2, 2024

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

@Linkgoron
Copy link
Member Author

Linkgoron commented Apr 2, 2024

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.

sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
The connection interval should close when httpsServer.close is called
similarly to how it gets cleared when httpServer.close is called.

Fixes: nodejs/node#48373
PR-URL: nodejs/node#48383
Backport-PR-URL: nodejs/node#50194
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
The connection interval should close when httpsServer.close is called
similarly to how it gets cleared when httpServer.close is called.

Fixes: nodejs/node#48373
PR-URL: nodejs/node#48383
Backport-PR-URL: nodejs/node#50194
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported-to-v18.x PRs backported to the v18.x-staging branch. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. http Issues or PRs related to the http subsystem. https Issues or PRs related to the https subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

https.Server does not call http.close