Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

resource leak in http.js #2069

Closed
quexer opened this issue Nov 11, 2011 · 9 comments
Closed

resource leak in http.js #2069

quexer opened this issue Nov 11, 2011 · 9 comments

Comments

@quexer
Copy link

quexer commented Nov 11, 2011

Here're codes from "Agent.prototype.removeSocket" in http.js

...

  if (this.sockets[name]) {
    var index = this.sockets[name].indexOf(s);
    if (index !== -1) {
      this.sockets[name].splice(index, 1);
    }
  } else if (this.sockets[name] && this.sockets[name].length === 0) {
    // don't leak
    delete this.sockets[name];
    delete this.requests[name];
  }

...

The "else if" branch is unreachable.

@koichik
Copy link

koichik commented Nov 29, 2011

@quexer: Thanks for the report, good catch!

Can someone review koichik/node@aeaa36a?

@bnoordhuis
Copy link
Member

@koichik: LGTM

@koichik
Copy link

koichik commented Dec 26, 2011

@bnoordhuis Thanks, merging.

@koichik
Copy link

koichik commented Dec 26, 2011

@quexer Thanks for the report, fixed in 7aa5924.

@quexer
Copy link
Author

quexer commented Dec 27, 2011

@koichik thanks

@ry
Copy link

ry commented Dec 27, 2011

thanks @koichik

isaacs added a commit to isaacs/node-v0.x-archive that referenced this issue Jan 6, 2012
* Upgrade V8 to 3.6.6.15

* Upgrade npm to 1.1.0-beta-10 (isaacs)

* many doc updates (Ben Noordhuis, Jeremy Martin, koichik, Dave Irvine,
  Seong-Rak Choi, Shannen, Adam Malcontenti-Wilson, koichik)

* nodejs#2438 segfault in node v0.6.6

* dgram, timers: fix memory leaks (Ben Noordhuis, Yoshihiro Kukuchi)

* repl: fix repl.start not passing the `ignoreUndefined` arg (Damon Oehlman)

* nodejs#1980: Socket.pause null reference when called on a closed Stream (koichik)

* nodejs#2263: XMLHttpRequest piped in a writable file stream hang (koichik)

* nodejs#2069: http resource leak (koichik)

* buffer.readInt global pollution fix (Phil Sung)

* timers: fix performance regression (Ben Noordhuis)

* nodejs#2308, nodejs#2246: node swallows openssl error on request (koichik)

* nodejs#2114: timers: remove _idleTimeout from item in .unenroll() (James Hartig)

* nodejs#2379: debugger: Request backtrace w/o refs (Fedor Indutny)

* simple DTrace ustack helper (Dave Pacheco)

* crypto: rewrite HexDecode without snprintf (Roman Shtylman)

* crypto: add SecureContext.clearOptions() method (Ben Noordhuis)

* crypto: don't ignore DH init errors (Ben Noordhuis)
isaacs added a commit that referenced this issue Jan 7, 2012
* V8 hash collision fix (Breaks MIPS) (Bert Belder, Erik Corry)

* Upgrade V8 to 3.6.6.15

* Upgrade npm to 1.1.0-beta-10 (isaacs)

* many doc updates (Ben Noordhuis, Jeremy Martin, koichik, Dave Irvine,
  Seong-Rak Choi, Shannen, Adam Malcontenti-Wilson, koichik)

* Fix segfault in node_http_parser.cc

* dgram, timers: fix memory leaks (Ben Noordhuis, Yoshihiro Kukuchi)

* repl: fix repl.start not passing the `ignoreUndefined` arg (Damon Oehlman)

* #1980: Socket.pause null reference when called on a closed Stream (koichik)

* #2263: XMLHttpRequest piped in a writable file stream hang (koichik)

* #2069: http resource leak (koichik)

* buffer.readInt global pollution fix (Phil Sung)

* timers: fix performance regression (Ben Noordhuis)

* #2308, #2246: node swallows openssl error on request (koichik)

* #2114: timers: remove _idleTimeout from item in .unenroll() (James Hartig)

* #2379: debugger: Request backtrace w/o refs (Fedor Indutny)

* simple DTrace ustack helper (Dave Pacheco)

* crypto: rewrite HexDecode without snprintf (Roman Shtylman)

* crypto: don't ignore DH init errors (Ben Noordhuis)
@lloyd
Copy link

lloyd commented Jan 7, 2012

Sorry for being lazy, but would this patch affect leak seen when agent: false, or is it only pertinent to http requests performed when the global connection pooling "agent" is in use?

@bnoordhuis
Copy link
Member

@lloyd: This patch only affects connection pooling. If you experience resource leaks with {agent:false}, can you open an issue with a test case?

@lloyd
Copy link

lloyd commented Jan 8, 2012

thanks @bnoordhuis, I'll try to distill a minimal test case and will open a diff bug if I succeed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants