http: don't emit timeout twice #4782

egirshov · 2013-02-15T15:43:58Z

If emitTimeout is passed to deferred setTimeout call, it is considered
as user's callback function and attached to 'timeout' event.
Thus single socket's timeout event might cause two (or even three)
events to be emitted by ClientRequest.

This is insignificant if request.setTimeout() method is used, which does this.once(), however could be noticed with code like this:

request.setTimeout(value);
request.on('timeout', callback);

This adds a proxy for bytesWritten to the tls.CryptoStream. This change makes the connection object more similar between HTTP and HTTPS requests in an effort to avoid confusion. See issue #4650 for more background information.

Fix issue where SlowBuffers couldn't be passed as target to Buffer copy(). Also included checks to see if Argument parameters are defined before assigning their values. This offered ~3x's performance gain.

Argument checks were simplified by setting all undefined/NaN or out of bounds values equal to their defaults. Also copy() tests had a flaw that each buffer had the same bit pattern at the same offset. So even if the copy failed, the bit-by-bit comparison would have still been true. This was fixed by filling each buffer with a unique value before copy operations.

Changed types of errors thrown to be more indicative of what the error represents. Also removed a few unnecessary uses of the v8 fully quantified typename.

* Omit ToObject() call. Buffer::Data() and Buffer::Length() know how to deal with Values. * Don't check if the argument is undefined because it realistically never is and undefined->integer coercion achieves the same thing.

@rvagg

mainly to allow native addons to export single functions on rather than being restricted to operating on an existing object. Init functions now receive exports as the first argument, like before, but also the module object as the second argument, if they support it. Related to #4634 cc: @rvagg

mainly to allow native addons to export single functions on `exports` rather than being restricted to operating on an existing `exports` object. added link to addons repo in docs

Before sending handle to another process using uv_write2(), it should be referenced to prevent it from being GCed before AfterWrite() will be called. see #4599

It's segfaulting in release mode and asserting in debug mode: # # Fatal error in ../../deps/v8/src/api.h, line 297 # CHECK(allow_empty_handle || that != __null) failed # This reverts commit 99f0b02.

Revert commit 7f2a78b and fix using empty symbol handle.

If the end argument is omitted or not a number, make it default to the end of the buffer, not zero. Ideally, it should not matter what it defaults to because the JS shim in lib/buffer.js should handle that but there are still several places in node.js core that secrete SlowBuffers, hence Buffer::Copy() gets called without going through Buffer.prototype.copy() first.

Fix the following OOM error in pummel/test-net-connect-memleak and pummel/test-tls-connect-memleak: FATAL ERROR: CALL_AND_RETRY_0 Allocation failed - process out of memory Commit v8/v8@91afd39 increases the size of the deoptimization table to the extent that a 64M float array pushes it over the brink. Switch to SMIs so it stays below the limit. pummel/test-net-connect-memleak is still failing albeit with a different error this time. Needs further investigation. === release test-net-connect-memleak === Path: pummel/test-net-connect-memleak -64 kB reclaimed assert.js:102 throw new assert.AssertionError({ ^ AssertionError: false == true at done [as _onTimeout] (/home/bnoordhuis/src/nodejs/master/ test/pummel/test-net-connect-memleak.js:48:3) at Timer.listOnTimeout [as ontimeout] (timers.js:110:15) at process._makeCallback (node.js:306:20)

The test times out when the upstream DNS resolver takes too long to respond. See #4672.

TCPWrap::Initialize() and PipeWrap::Initialize() should be called before any data will be read from received socket. But, because of lazy initialization of these bindings, Initialize() method isn't called. Init bindings manually upon socket receiving. See #4669

The refactor in b43e544 to use stream.push() in Transform inadvertently caused it to immediately consume all the written data, regardless of whether or not the readable side was being consumed. Only pull data through the _transform() process when the readable side is being consumed. Fix #4667

The better to reduce the hidden classes

Doing this in net.Socket constructor has much more overhead, and error is actually may happen before the construction of socket object.

When a datagram socket hasn't been bound yet, node will defer `send()` operations until binding has completed. Before this patch a `listening` listener would be installed every time `send` was called. This triggered an EventEmitter leak warning when more than 10 packets were sent in a tight loop. Therefore switch to using a single `listening` listener, and use an array to enqueue outbound packets.

Always defer the _write callback. The optimization here was only relevant in some oddball edge cases that we don't actually care about. Our benchmarks confirm that just always deferring the Socket._write cb is perfectly fine to do, and in some cases, even slightly more performant.

This commit breaks simple/test-stream2-stderr-sync. Need to figure out a better way, or just accept that `(function W(){stream.write(b,W)})()` is going to be noisy. People should really be using the `'drain'` event for this use-case anyway. This reverts commit 02f7d1b.

Make lines ending \r\n emit one 'line' event, not two (where the second one is an empty string). This adds a new keypress name: 'return' (as in: 'carriage return'). Fixes #3305.

Fixes #3305

Make the casing consistent with the other os.* functions but keep os.tmpDir() around as an alias.

Make sure the argument passed is a string. Also use typeof === function check instead of isArray().

Remove unecessary splice for single listener events. Add type check for "type" argument.

By making sure the _events is always an object there is one less check that needs to be performed by emit. Use undefined instead of null. typeof checks are a lot faster than isArray. There are a few places where the this._events check cannot be removed because it is possible for the user to call those methods after using utils.extend to create their own EventEmitter, but before it has actually been instantiated.

Check both passed args to addListener. Place var at beginning.

Cleanup check logic. Place vars at top. Remove PROCESS.

Also cleanup unnecessary use of "self" since it will always be called using .apply() from emit.

The file has a long name that's apparently impossible to remove with `git clean` on Windows.

The `heat` tool that gathers NPM source files wasn't getting called. Closes #4896

The stock writable stream "write after end" message is overly vague, if you have clearly not called end() yourself yet. When we receive a FIN from the other side, and call destroySoon() as a result, then generate an EPIPE error (which is what would happen if you did actually write to the socket), with a message explaining what actually happened.

Conflicts: doc/api/http.markdown test/simple/test-crypto.js

The previous commit did not handle the case when the event type is 'error', since that is checked before reading the handler.

Calling end(data) calls write(data). Doing this after end should raise a 'write after end' error. However, because end() calls were previously ignored on already ended streams, this error was confusingly suppressed, even though the data never is written, and cannot get to the other side.

downgrade TO 3.14

It's breaking ~22 tests, Needs further investigation. This reverts commit 5222d19.

The try/catch in repl.js keeps any active domain from catching the error. Since the domain may not even be enterd until the code is run, it's not possible to avoid the try/catch, so emit on the domain when an error is thrown.

Calling end(data) calls write(data). Doing this after end should raise a 'write after end' error. However, because end() calls were previously ignored on already ended streams, this error was confusingly suppressed, even though the data never is written, and cannot get to the other side. This is a re-hash of 5222d19, but without assuming that the data passed to end() is valid, and thus breaking a bunch of tests.

Fix #4133, bringing the cluster worker API more in line with the child process API.

isaacs · 2013-03-04T07:41:31Z

Can you add a test?

A typo in the variable name makes it throw a ReferenceError instead of the expected "Unknown type" error when dns.resolve() is passed a bad record type argument. Fixes the following exception: ReferenceError: type is not defined at Object.exports.resolve (dns.js:189:40) at /Users/bnoordhuis/src/master/test/simple/test-c-ares.js:48:9 <snip>

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [nodejs#4682](nodejs/node#4682) * Previously deprecated Buffer APIs are removed [nodejs#5048](nodejs/node#5048), [nodejs#4594](nodejs/node#4594) * Improved error handling [nodejs#4514](nodejs/node#4514) * Cluster * Worker emitted as first argument in 'message' event [nodejs#5361](nodejs/node#5361). * Crypto * Improved error handling [nodejs#3100](nodejs/node#3100), [nodejs#5611](nodejs/node#5611) * Simplified Certificate class bindings [nodejs#5382](nodejs/node#5382) * Improved control over FIPS mode [nodejs#5181](nodejs/node#5181) * pbkdf2 digest overloading is deprecated [nodejs#4047](nodejs/node#4047) * Dependencies * Reintroduce shared c-ares build support [nodejs#5775](nodejs/node#5775). * V8 updated to 5.0.71.31 [nodejs#6111](nodejs/node#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [nodejs#4921](nodejs/node#4921). * Domains * Clear stack when no error handler [nodejs#4659](nodejs/node#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [nodejs#3594](nodejs/node#3594) * FS apis can now accept and return paths as Buffers [nodejs#5616](nodejs/node#5616). * Error handling and type checking improvements [nodejs#5616](nodejs/node#5616), [nodejs#5590](nodejs/node#5590), [nodejs#4518](nodejs/node#4518), [nodejs#3917](nodejs/node#3917). * fs.read's string interface is deprecated [nodejs#4525](nodejs/node#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [nodejs#4557](nodejs/node#4557). * Modules * Current directory is now prioritized for local lookups [nodejs#5689](nodejs/node#5689) * Symbolic links are preserved when requiring modules [nodejs#5950](nodejs/node#5950) * Net * DNS hints no longer implicitly set [nodejs#6021](nodejs/node#6021). * Improved error handling and type checking [nodejs#5981](nodejs/node#5981), [nodejs#5733](nodejs/node#5733), [nodejs#2904](nodejs/node#2904) * Path * Improved type checking [nodejs#5348](nodejs/node#5348). * Process * Introduce process warnings API [nodejs#4782](nodejs/node#4782). * Throw exception when non-function passed to nextTick [nodejs#3860](nodejs/node#3860). * Readline * Emit key info unconditionally [nodejs#6024](nodejs/node#6024) * REPL * Assignment to `_` will emit a warning. [nodejs#5535](nodejs/node#5535) * Timers * Fail early when callback is not a function [nodejs#4362](nodejs/node#4362) * TLS * Rename 'clientError' to 'tlsClientError' [nodejs#4557](nodejs/node#4557) * SHA1 used for sessionIdContext [nodejs#3866](nodejs/node#3866) * TTY * Previously deprecated setRawMode wrapper is removed [nodejs#2528](nodejs/node#2528). * Util * Changes to Error object formatting [nodejs#4582](nodejs/node#4582). * Windows * Windows XP and Vista are no longer supported [nodejs#5167](nodejs/node#5167), [nodejs#5167](nodejs/node#5167).

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [nodejs#4682](nodejs/node#4682) * Previously deprecated Buffer APIs are removed [nodejs#5048](nodejs/node#5048), [nodejs#4594](nodejs/node#4594) * Improved error handling [nodejs#4514](nodejs/node#4514) * Cluster * Worker emitted as first argument in 'message' event [nodejs#5361](nodejs/node#5361). * Crypto * Improved error handling [nodejs#3100](nodejs/node#3100), [nodejs#5611](nodejs/node#5611) * Simplified Certificate class bindings [nodejs#5382](nodejs/node#5382) * Improved control over FIPS mode [nodejs#5181](nodejs/node#5181) * pbkdf2 digest overloading is deprecated [nodejs#4047](nodejs/node#4047) * Dependencies * Reintroduce shared c-ares build support [nodejs#5775](nodejs/node#5775). * V8 updated to 5.0.71.31 [nodejs#6111](nodejs/node#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [nodejs#4921](nodejs/node#4921). * Domains * Clear stack when no error handler [nodejs#4659](nodejs/node#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [nodejs#3594](nodejs/node#3594) * FS apis can now accept and return paths as Buffers [nodejs#5616](nodejs/node#5616). * Error handling and type checking improvements [nodejs#5616](nodejs/node#5616), [nodejs#5590](nodejs/node#5590), [nodejs#4518](nodejs/node#4518), [nodejs#3917](nodejs/node#3917). * fs.read's string interface is deprecated [nodejs#4525](nodejs/node#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [nodejs#4557](nodejs/node#4557). * Modules * Current directory is now prioritized for local lookups [nodejs#5689](nodejs/node#5689) * Symbolic links are preserved when requiring modules [nodejs#5950](nodejs/node#5950) * Net * DNS hints no longer implicitly set [nodejs#6021](nodejs/node#6021). * Improved error handling and type checking [nodejs#5981](nodejs/node#5981), [nodejs#5733](nodejs/node#5733), [nodejs#2904](nodejs/node#2904) * OS X * MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7 [nodejs#6402](nodejs/node#6402). * Path * Improved type checking [nodejs#5348](nodejs/node#5348). * Process * Introduce process warnings API [nodejs#4782](nodejs/node#4782). * Throw exception when non-function passed to nextTick [nodejs#3860](nodejs/node#3860). * Readline * Emit key info unconditionally [nodejs#6024](nodejs/node#6024) * REPL * Assignment to `_` will emit a warning. [nodejs#5535](nodejs/node#5535) * Timers * Fail early when callback is not a function [nodejs#4362](nodejs/node#4362) * TLS * Rename 'clientError' to 'tlsClientError' [nodejs#4557](nodejs/node#4557) * SHA1 used for sessionIdContext [nodejs#3866](nodejs/node#3866) * TTY * Previously deprecated setRawMode wrapper is removed [nodejs#2528](nodejs/node#2528). * Util * Changes to Error object formatting [nodejs#4582](nodejs/node#4582). * Windows * Windows XP and Vista are no longer supported [nodejs#5167](nodejs/node#5167), [nodejs#5167](nodejs/node#5167).

isaacs and others added 30 commits January 24, 2013 10:12

test: Add transform objectMode test

6e05faa

Merge branch 'v0.9.8-release'

42309a3

Now working on v0.9.9

f64742e

Add bytesWritten to tls.CryptoStream

595b597

This adds a proxy for bytesWritten to the tls.CryptoStream. This change makes the connection object more similar between HTTP and HTTPS requests in an effort to avoid confusion. See issue #4650 for more background information.

doc: Connecting debugger to existing node process

83154aa

buffer: slow buffer copy compatibility fix

16bbecc

Fix issue where SlowBuffers couldn't be passed as target to Buffer copy(). Also included checks to see if Argument parameters are defined before assigning their values. This offered ~3x's performance gain.

buffer: error and misc cleanup

cbe3941

Changed types of errors thrown to be more indicative of what the error represents. Also removed a few unnecessary uses of the v8 fully quantified typename.

buffer: remove minor Buffer::Copy deoptimizations

00b4b7b

* Omit ToObject() call. Buffer::Data() and Buffer::Length() know how to deal with Values. * Don't check if the argument is undefined because it realistically never is and undefined->integer coercion achieves the same thing.

doc: NODE_MODULE() to pass full module to addons

f210710

mainly to allow native addons to export single functions on `exports` rather than being restricted to operating on an existing `exports` object. added link to addons repo in docs

doc: fix line wrapping in addons.markdown

bdc7251

stream_wrap: reference handle before uv_write2

99f0b02

Before sending handle to another process using uv_write2(), it should be referenced to prevent it from being GCed before AfterWrite() will be called. see #4599

Revert "stream_wrap: reference handle before uv_write2"

7f2a78b

It's segfaulting in release mode and asserting in debug mode: # # Fatal error in ../../deps/v8/src/api.h, line 297 # CHECK(allow_empty_handle || that != __null) failed # This reverts commit 99f0b02.

stream_wrap: reference handle before uv_write2

0972acb

Revert commit 7f2a78b and fix using empty symbol handle.

test: move simple/test-http-dns-fail to test/internet

a39f669

The test times out when the upstream DNS resolver takes too long to respond. See #4672.

http: Don't dump twice

faf7860

net: Initialize _connection, _handle in Socket ctor

4c78a52

The better to reduce the hidden classes

child_process: move binding init in constructor

c13354e

Doing this in net.Socket constructor has much more overhead, and error is actually may happen before the construction of socket object.

cluster: support datagram sockets

5e7e51c

readline: make \r\n emit one 'line' event

9bd9c54

Make lines ending \r\n emit one 'line' event, not two (where the second one is an empty string). This adds a new keypress name: 'return' (as in: 'carriage return'). Fixes #3305.

readline: treat bare \r as a line ending

60f18ed

Fixes #3305

os: rename tmpDir() to tmpdir() for consistency

3fe6aba

Make the casing consistent with the other os.* functions but keep os.tmpDir() around as an alias.

trevnorris and others added 21 commits March 1, 2013 17:36

events: type check listeners

aba03eb

Make sure the argument passed is a string. Also use typeof === function check instead of isArray().

events: removeListener add checks and cleanup

b3ea844

Remove unecessary splice for single listener events. Add type check for "type" argument.

events: additional type check for addListener

1ccc6fb

Check both passed args to addListener. Place var at beginning.

events: emit cleanup

e1ac2ef

Cleanup check logic. Place vars at top. Remove PROCESS.

events: add type checks to once

d1b4dcd

Also cleanup unnecessary use of "self" since it will always be called using .apply() from emit.

test: unlink temp file at test start

93156a6

The file has a long name that's apparently impossible to remove with `git clean` on Windows.

windows/msi: fix msi build issue with WiX 3.7/3.8

47e1150

The `heat` tool that gathers NPM source files wasn't getting called. Closes #4896

net: s/closed/ended/ in write-after-fin message

4ac73d2

Merge remote-tracking branch 'origin/v0.8'

2d51036

Conflicts: doc/api/http.markdown test/simple/test-crypto.js

events: Handle emit before constructor call

d345c11

events: Handle emit('error') before ctor

63edde0

The previous commit did not handle the case when the event type is 'error', since that is checked before reading the handler.

doc: Typo in ChangeLog

f5d8496

downgrade TO 3.14

Revert "stream: Writable.end(chunk) after end is an error"

80ea63b

It's breaking ~22 tests, Needs further investigation. This reverts commit 5222d19.

make repl compatible with domains

22aa767

The try/catch in repl.js keeps any active domain from catching the error. Since the domain may not even be enterd until the code is run, it's not possible to avoid the try/catch, so emit on the domain when an error is thrown.

doc: Add crypto.pseudoRandomBytes, fix typo

0c57b31

cluster: Rename destroy() to kill(signal=SIGTERM)

e428bb7

Fix #4133, bringing the cluster worker API more in line with the child process API.

link to LevelUP modules wiki page, not level-hooks

906f824

upsuper and others added 3 commits March 4, 2013 16:20

http: fix multile timeout events

7e5469d

test: add a test on single timeout event

d863650

egirshov closed this Mar 5, 2013

egirshov mentioned this pull request Mar 5, 2013

Fix multiple timeout events #4920

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: don't emit timeout twice #4782

http: don't emit timeout twice #4782

egirshov commented Feb 15, 2013

isaacs commented Mar 4, 2013

http: don't emit timeout twice #4782

http: don't emit timeout twice #4782

Conversation

egirshov commented Feb 15, 2013

isaacs commented Mar 4, 2013