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.

Sign up for GitHub

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

Jump to bottom

http2: close idle connections when allowHTTP1 is true #51569

Merged
merged 1 commit into from Feb 1, 2024
Merged

http2: close idle connections when allowHTTP1 is true #51569

merged 1 commit into from Feb 1, 2024

Conversation

xsbchen
Copy link
Contributor

@xsbchen xsbchen commented Jan 26, 2024

Fixes: #51493

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http2
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run. labels Jan 26, 2024
@marco-ippolito
Copy link
Member

Can you write a test please?

@xsbchen
Copy link
Contributor Author

xsbchen commented Jan 26, 2024

Can you write a test please?

sure, will do later

mcollina
mcollina approved these changes Jan 30, 2024
View reviewed changes
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

@xsbchen
Copy link
Contributor Author

xsbchen commented Jan 31, 2024

@marco-ippolito test case added, plz help to review

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

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

@nodejs-github-bot
Copy link
Collaborator

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

@mcollina mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 1, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 1, 2024
@nodejs-github-bot nodejs-github-bot merged commit d1114c4 into nodejs:main Feb 1, 2024
55 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in d1114c4

rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Feb 9, 2024
@xsbchen @rdw-msft
http2: close idle connections when allowHTTP1 is true
342c51a 
Fixes: nodejs#51493
PR-URL: nodejs#51569
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
targos pushed a commit that referenced this pull request Feb 15, 2024
@xsbchen @targos
http2: close idle connections when allowHTTP1 is true
682951a 
Fixes: #51493
PR-URL: #51569
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
marco-ippolito pushed a commit to marco-ippolito/node that referenced this pull request Feb 19, 2024
@xsbchen @marco-ippolito
http2: close idle connections when allowHTTP1 is true
d06de96 
Fixes: nodejs#51493
PR-URL: nodejs#51569
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
@marco-ippolito marco-ippolito mentioned this pull request Mar 1, 2024
Merged
richardlau pushed a commit that referenced this pull request Mar 25, 2024
@xsbchen @richardlau
http2: close idle connections when allowHTTP1 is true
8a990bc 
Fixes: #51493
PR-URL: #51569
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
richardlau pushed a commit that referenced this pull request Mar 25, 2024
@xsbchen @richardlau
http2: close idle connections when allowHTTP1 is true
b560bfb 
Fixes: #51493
PR-URL: #51569
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
@richardlau richardlau mentioned this pull request Mar 25, 2024
Merged
eugene1g added a commit to eugene1g/node-http2-timer that referenced this pull request Apr 26, 2024
@eugene1g
http2: fix excessive CPU usage when using allowHTTP1=true
2ea4b2a 
When Http2SecureServer is configured with `allowHTTP1=true`, it calls
`setupConnectionsTracking` to start monitoring for idle HTTP1
connections. `setupConnectionsTracking` expects to have
`this.connectionsCheckingInterval` property defined, but it does not
exist on `Http2SecureServer`. This `undefined` value is passed to
`setInterval`, which results in `checkConnections` being called on
every tick, creating significant additional load on the server CPU.
The fix is to define `this.connectionsCheckingInterval` on the
Http2SecureServer instance.

Refs: nodejs#51569
@eugene1g eugene1g mentioned this pull request Apr 26, 2024
Merged
nodejs-github-bot pushed a commit that referenced this pull request Apr 28, 2024
@eugene1g
http2: fix excessive CPU usage when using allowHTTP1=true
2bdd670 
When Http2SecureServer is configured with `allowHTTP1=true`, it calls
`setupConnectionsTracking` to start monitoring for idle HTTP1
connections. `setupConnectionsTracking` expects to have
`this.connectionsCheckingInterval` property defined, but it does not
exist on `Http2SecureServer`. This `undefined` value is passed to
`setInterval`, which results in `checkConnections` being called on
every tick, creating significant additional load on the server CPU.
The fix is to define `this.connectionsCheckingInterval` on the
Http2SecureServer instance.

Refs: #51569
PR-URL: #52713
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
aduh95 pushed a commit that referenced this pull request Apr 29, 2024
@eugene1g @aduh95
http2: fix excessive CPU usage when using allowHTTP1=true
d5ecb6c 
When Http2SecureServer is configured with `allowHTTP1=true`, it calls
`setupConnectionsTracking` to start monitoring for idle HTTP1
connections. `setupConnectionsTracking` expects to have
`this.connectionsCheckingInterval` property defined, but it does not
exist on `Http2SecureServer`. This `undefined` value is passed to
`setInterval`, which results in `checkConnections` being called on
every tick, creating significant additional load on the server CPU.
The fix is to define `this.connectionsCheckingInterval` on the
Http2SecureServer instance.

Refs: #51569
PR-URL: #52713
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
marco-ippolito pushed a commit that referenced this pull request May 2, 2024
@eugene1g @marco-ippolito
http2: fix excessive CPU usage when using allowHTTP1=true
a714c55 
When Http2SecureServer is configured with `allowHTTP1=true`, it calls
`setupConnectionsTracking` to start monitoring for idle HTTP1
connections. `setupConnectionsTracking` expects to have
`this.connectionsCheckingInterval` property defined, but it does not
exist on `Http2SecureServer`. This `undefined` value is passed to
`setInterval`, which results in `checkConnections` being called on
every tick, creating significant additional load on the server CPU.
The fix is to define `this.connectionsCheckingInterval` on the
Http2SecureServer instance.

Refs: #51569
PR-URL: #52713
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
marco-ippolito pushed a commit that referenced this pull request May 3, 2024
@eugene1g @marco-ippolito
http2: fix excessive CPU usage when using allowHTTP1=true
3bcd683 
When Http2SecureServer is configured with `allowHTTP1=true`, it calls
`setupConnectionsTracking` to start monitoring for idle HTTP1
connections. `setupConnectionsTracking` expects to have
`this.connectionsCheckingInterval` property defined, but it does not
exist on `Http2SecureServer`. This `undefined` value is passed to
`setInterval`, which results in `checkConnections` being called on
every tick, creating significant additional load on the server CPU.
The fix is to define `this.connectionsCheckingInterval` on the
Http2SecureServer instance.

Refs: #51569
PR-URL: #52713
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@Qard Qard Qard approved these changes

@marco-ippolito marco-ippolito marco-ippolito approved these changes

Assignees
No one assigned
Labels
http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

HTTP/2 server.close() is broken with "allowHTTP1" mode
5 participants
@xsbchen @nodejs-github-bot @marco-ippolito @mcollina @Qard