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

http: remove listeners from free sockets #29259

Closed
wants to merge 2 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Aug 21, 2019

This is a continuation on #29245 made in a separate PR to make #29245 easier to fast track if desired.

Some more listener cleanup when sockets are detached from the request object and moved into the socket pool.

I'm not sure, but this looks like it might actually be a bug?

Checklist
  • 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 nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Aug 21, 2019
@ronag ronag force-pushed the http-free-listener branch 2 times, most recently from 81282c4 to 94a1aa9 Compare August 21, 2019 22:25
@ronag ronag changed the title Http free listener http: remove listeners from free sockets Aug 21, 2019
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM, but this needs a rebase

@ronag
Copy link
Member Author

ronag commented Aug 25, 2019

Rebased

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 25, 2019
@nodejs-github-bot
Copy link
Collaborator

@Trott Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 9, 2019
@ronag
Copy link
Member Author

ronag commented Sep 23, 2019

rebased

@ronag ronag force-pushed the http-free-listener branch 2 times, most recently from 05988bf to f9486be Compare September 23, 2019 07:47
Reduced memory usage by ensuring free sockets don't have extra
listeners while in the pool.
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 baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v10.x labels Nov 15, 2019
@ronag
Copy link
Member Author

ronag commented Dec 29, 2019

@Trott: Can this land?

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 30, 2019
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 2, 2020

BridgeAR pushed a commit that referenced this pull request Jan 3, 2020
Reduced memory usage by ensuring free sockets don't have extra
listeners while in the pool.

PR-URL: #29259
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeAR
Copy link
Member

BridgeAR commented Jan 3, 2020

Landed in 7c70dbb 🎉

@BridgeAR BridgeAR closed this Jan 3, 2020
BridgeAR pushed a commit that referenced this pull request Jan 3, 2020
Reduced memory usage by ensuring free sockets don't have extra
listeners while in the pool.

PR-URL: #29259
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
targos pushed a commit that referenced this pull request Jan 14, 2020
Reduced memory usage by ensuring free sockets don't have extra
listeners while in the pool.

PR-URL: #29259
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Jan 14, 2020
Reduced memory usage by ensuring free sockets don't have extra
listeners while in the pool.

PR-URL: #29259
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
Reduced memory usage by ensuring free sockets don't have extra
listeners while in the pool.

PR-URL: #29259
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
@targos targos removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Sep 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants