Rename errno, DRY errnoException() #2492

Closed
wants to merge 19 commits into
from

Projects

None yet

10 participants

@bnoordhuis
Node.js Foundation member
  1. Rename errno to __errno to avoid clashing with user code. Maybe we should move it into the process object.

  2. Move errnoException() into util.

Passes all tests. Patches are against v0.6.

Review / thoughts, please.

ry and others added some commits Jan 4, 2012
@ry ry Add another test to test-http-parser-bad-ref.js demoing #2438 2cde498
@ry ry Potential fix for #2438
- Save StringPtr if the header hasn't been completely received yet after one
  packet.
- Add one to num_fields and num_values. They were actually one less than the
  number of fields and values.
- Remove always_inline makes debugging difficult, and has negligible
  performance benefits.
f3da6c6
@ry ry Update address in CLA 3452477
@koichik koichik http: use `self` insted of `this` baebd30
@bnoordhuis bnoordhuis docs: mention that python 2.6 or 2.7 is required 760928b
@jbergstroem
Node.js Foundation member

Hm, I thought the idea of using pprint would allow for 2.5. If we are to use >=2.6, we should switch to json since pprint has its caveats (quoting)

Node.js Foundation member

python 2.5 will work for the foreseeable future but I'm anticipating the day when a gyp update breaks 2.5 compatibility.

isaacs and others added some commits Jan 4, 2012
@isaacs isaacs npm@1.1.0-beta-10 78dbb4b
@koichik koichik http: fix ServerResponse does not emit 'close'
Refs #2453.
dd9593c
@piscisaureus piscisaureus Land hash collision fix for V8 3.6 by Erik Corry.
- If V8 snapshots are enabled then the hash is only randomized at build time.
- Breaks MIPS

---
Backport hash collision workaround to 3.6.
This is made up of 9956, 10351, 10338 and 10330.
This change bakes the string hash key into the snapshot, so
it is determined at build time for shapshot configs.
Review URL: http://codereview.chromium.org/9124004
4a899c9
@ry ry fix test-sys for hash randomization
broken in 4a899c9
8bd80f4
@ry ry Revert "crypto: add SecureContext.clearOptions() method"
API addition needs to go in master. Also openssl-0.9.8k doesn't have
SSL_CTX_clear_options().

This reverts commit 6f8839d.
be67fa7
@bnoordhuis
Node.js Foundation member

Two things:

  1. I don't consider this an API change but a bug fix. You cannot unset options with SSL_CTX_set_options() since it ORs the new options with the old.

  2. It cannot have failed with 0.9.8k since I test with 0.9.8k (both stock and Ubuntu-ized).

I don't want to revert this, it's one part of a two-pronged attack on mitigating TLS renegotiation attacks. If some versions of openssl lack SSL_CTX_clear_options() (it was added in response to CVE-2009-3555), I'd rather have a feature check than nothing at all.

ry and others added some commits Jan 7, 2012
@ry ry support nosnapshot in vcbuild.bat bca88b2
@isaacs isaacs 2012.01.06, Version 0.6.7 (stable)
* 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)
d5a189a
@isaacs isaacs Remove snapshot from Mac binary build d84a6ba
@isaacs isaacs Now working on v0.6.8 ff4096f
@koichik koichik docs: small changes. 57653ad
@koichik koichik docs: fix ChangeLog 9ef3c62
@koichik

@isaacs - Can you please correct Node blog?

Oh, whoops! Sorry about that!

Thanks for the quick response!

bnoordhuis added some commits Jan 8, 2012
@bnoordhuis bnoordhuis build: honour the PYTHON environment variable
Overrides the path to the python binary. Defaults to `python`.
472a72d
@bnoordhuis bnoordhuis DRY errnoException()
Move errnoException() into util, remove duplicated code.
b174dcf
@bnoordhuis bnoordhuis Rename `errno` global to `__errno`.
Naming it `errno` was a mistake, the risk of name clashes with user code is too
high.
53e5c67
@bnoordhuis
Node.js Foundation member

Sorry, something went wrong while submitting the PR. Please see #2493.

@bnoordhuis bnoordhuis closed this Jan 8, 2012
@vulkanr

i think "assignSocket" is called every time on keep-alive connections. The "close" event listeners will just keep ramping up until hitting the listener limit.

Node.js Foundation member

Do you have a test case I can try? The logic seems okay to me and I see a removal for every added 'close' listener.

plain vanilla handler:

app.get('/test', function(req,res) {
res.end();
});

  1. every "curl" (using localhost) will result in an exception (actually showing a bigger problem than i reported): curl -v http://localhost:8888/test

Uncaught TypeError: Cannot call method 'emit' of null
at Socket.onServerResponseClose (http.js:780:21)
at Socket.emit (events.js:88:20)
at Array. (net.js:320:10)
at EventEmitter._tickCallback (node.js:192:40)

  1. to show the problem i reported - simply issue that "curl" with multiple urls (default in curl is keep-alive). example: curl -v http://localhost:8888/test http://localhost:8888/test http://localhost:8888/test http://localhost:8888/test http://localhost:8888/test http://localhost:8888/test http://localhost:8888/test http://localhost:8888/test http://localhost:8888/test http://localhost:8888/test http://localhost:8888/test

server warning/exception:
(node) warning: possible EventEmitter memory leak detected. 11 listeners added. Use emitter.setMaxListeners() to increase limit.
Trace:
at Socket. (events.js:139:15)
at ServerResponse.assignSocket (http.js:786:10)
at HTTPParser.onIncoming (http.js:1454:11)
at HTTPParser.onHeadersComplete (http.js:102:31)
at Socket.ondata (http.js:1387:22)
at TCP.onread (net.js:354:27)

Node.js Foundation member

That's probably a bug in whatever library you're using. Below is a plain Node example that works fine with curl or ab -k:

require('http').createServer(function(req, res) {
  res.end();
}).listen(8000);

you are right. sorry for the trouble. it was an old piece of code which got included in my test. upgrade to 0.6.8 kicked the error in.

How did you resolve this? I've got the exact same error, but I'm not sure what old code would cause this.
edit: found the cause. It was the long-stack-traces module.

@rstuven

@vulkanr @kelnishi In my case, it was an interaction with long-stack-traces module too.

@digitalrinaldo digitalrinaldo referenced this pull request in tlrobinson/long-stack-traces Nov 9, 2012
Open

Is there an issue with Uncaught Type error #10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment