Skip to content

http: remove stale timeout listeners #9831

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
wants to merge 1 commit into from

Conversation

evantorrie
Copy link
Contributor

@evantorrie evantorrie commented Nov 28, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

http

Description of change

Fix memory leak #9268 introduced by ee7af01

Unifies handling of timeout set via http#request (i.e. prior to socket connect) with explicit ClientRequest.prototype.setTimeout. Previously, setTimeout would only apply if socket was writeable. Now, the timeout is applied regardless of whether socket is writeable, i.e. it applies to connect + write + read response combined.

Alternative solution to #9440

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Nov 28, 2016
@Fishrock123 Fishrock123 added the memory Issues and PRs related to the memory management or memory footprint. label Dec 8, 2016
@Fishrock123
Copy link
Contributor

@cjihrig @jasnell @evanlucas @bnoordhuis you all LGTM'd the alternative PR, is this one less favorable?

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

I wasn't aware this PR existed. :-) It has the benefit of creating fewer closures. That's never a bad thing.

@evanlucas
Copy link
Contributor

@evantorrie can you rebase this off of current master? Thanks!

@jasnell
Copy link
Member

jasnell commented Jan 6, 2017

#9440 landed. Closing this without landing

@jasnell jasnell closed this Jan 6, 2017
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
http Issues or PRs related to the http subsystem. memory Issues and PRs related to the memory management or memory footprint.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants