http_server: fix resume after socket close #2824

indutny · 2015-09-11T21:07:07Z

Socket resume may happen on a next tick, and in following scenario:

socket.resume()
socket._handle.close()
socket._handle = null;

The _resume will be invoked with empty ._handle property. There is
nothing bad about it, and we should just ignore the resume/pause
events in this case.

Same applies to the unconsuming of socket on adding data and/or
readable event listeners.

Fix: #2821

Socket resume may happen on a next tick, and in following scenario: 1. `socket.resume()` 2. `socket._handle.close()` 3. `socket._handle = null;` The `_resume` will be invoked with empty `._handle` property. There is nothing bad about it, and we should just ignore the `resume`/`pause` events in this case. Same applies to the unconsuming of socket on adding `data` and/or `readable` event listeners. Fix: nodejs#2821

indutny · 2015-09-11T21:07:32Z

cc @nodejs/collaborators @ChALkeR

ChALkeR · 2015-09-11T21:09:36Z

Testing.
cc @geek

ChALkeR · 2015-09-11T21:11:48Z

test/parallel/test-http-regr-gh-2821.js

+    port: common.PORT
+  });
+
+  const payload = new Array(1640).join('0123456789');


Imo a new Buffer(16384).fill(0) would be better here. Or with some other value that was causing this.

Sounds good, fixed.

geek · 2015-09-11T21:12:42Z

@indutny thanks for the quick turnaround 👍

ChALkeR · 2015-09-11T21:17:47Z

test/parallel/test-http-regr-gh-2821.js

+    port: common.PORT
+  });
+
+  const payload = new Buffer(16390);


Can you fill it with something, please? I would prefer if random memory regions were not passed through the network stack, even in the harmless tests.

Here you go, not sure how relevant it was, though ;)

The same applies to other tests — some of them fill buffers, some don't. But that is out of scope for now.

@indutny Thanks!

ChALkeR · 2015-09-11T21:28:36Z

The fix works for me (when applied on top of v3.3.0).
LGTM is CI passes.

cjihrig · 2015-09-11T21:31:23Z

LGTM pending CI. Kind of funny that the test doesn't actually assert anything. It just passes by not crashing :-)

Trott · 2015-09-11T22:32:17Z

If that's the case, the require('assert') can be removed. (Or not.)

silverwind · 2015-09-11T22:43:48Z

@Trott that could be material for another PR. There's a few tests that have unused local variables. I'll look into that.

Trott · 2015-09-11T22:46:14Z

@silverwind 👍 That subsequent PR might be able to include the no-unused-vars rule for eslint in the lint step too.

silverwind · 2015-09-11T22:47:34Z

@Trott yeah, that's the plan.
@indutny you might as well remove that require('assert') now ;)

ChALkeR · 2015-09-13T13:26:41Z

CI: https://ci.nodejs.org/job/node-test-pull-request/289/

Some ARM machines and FreeBSD failed.
ARM machines failed at git stage (CI bug), FreeBSD failed several test-*dgram* tests with Error: bind EADDRINUSE 0.0.0.0:12346 (probably a CI bug).

CI re-run: https://ci.nodejs.org/job/node-test-pull-request/290/

Fishrock123 · 2015-09-14T22:14:47Z

CI isn't so happy but it seems unrelated?

cc @indutny could we expedite this? :)

indutny · 2015-09-14T22:26:25Z

@Fishrock123 appears to be totally unrelated indeed. Please land it ;)

Socket resume may happen on a next tick, and in following scenario: 1. `socket.resume()` 2. `socket._handle.close()` 3. `socket._handle = null;` The `_resume` will be invoked with empty `._handle` property. There is nothing bad about it, and we should just ignore the `resume`/`pause` events in this case. Same applies to the unconsuming of socket on adding `data` and/or `readable` event listeners. Fix: #2821 PR-URL: #2824 Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

Fishrock123 · 2015-09-15T01:12:43Z

Landed as requested in 7ec0491

Socket resume may happen on a next tick, and in following scenario: 1. `socket.resume()` 2. `socket._handle.close()` 3. `socket._handle = null;` The `_resume` will be invoked with empty `._handle` property. There is nothing bad about it, and we should just ignore the `resume`/`pause` events in this case. Same applies to the unconsuming of socket on adding `data` and/or `readable` event listeners. Fix: #2821 PR-URL: #2824 Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

Notable changes: * buffer: - Buffers are now created in JavaScript, rather than C++. This increases the speed of buffer creation (Trevor Norris) nodejs#2866. - `Buffer#slice()` now uses `Uint8Array#subarray()` internally, increasing `slice()` performance (Karl Skomski) nodejs#2777. * fs: - `fs.utimes()` now properly converts numeric strings, `NaN`, and `Infinity` (Yazhong Liu) nodejs#2387. - `fs.WriteStream` now implements `_writev`, allowing for super-fast bulk writes (Ron Korving) nodejs#2167. * http: Fixed an issue with certain `write()` sizes causing errors when using `http.request()` (Fedor Indutny) nodejs#2824. * npm: Upgrade to version 2.14.3, see https://github.com/npm/npm/releases/tag/v2.14.3 for more details (Kat Marchán) nodejs#2822. * src: V8 cpu profiling no longer erroneously shows idle time (Oleksandr Chekhovskyi) nodejs#2324. * v8: Lateral upgrade to 4.5.103.33 from 4.5.103.30, contains minor fixes (Ali Ijaz Sheikh) nodejs#2870. - This fixes a previously known bug where some computed object shorthand properties did not work correctly (nodejs#2507). Refs: nodejs#2844 PR-URL: nodejs#2889

Notable changes: * buffer: - Buffers are now created in JavaScript, rather than C++. This increases the speed of buffer creation (Trevor Norris) #2866. - `Buffer#slice()` now uses `Uint8Array#subarray()` internally, increasing `slice()` performance (Karl Skomski) #2777. * fs: - `fs.utimes()` now properly converts numeric strings, `NaN`, and `Infinity` (Yazhong Liu) #2387. - `fs.WriteStream` now implements `_writev`, allowing for super-fast bulk writes (Ron Korving) #2167. * http: Fixed an issue with certain `write()` sizes causing errors when using `http.request()` (Fedor Indutny) #2824. * npm: Upgrade to version 2.14.3, see https://github.com/npm/npm/releases/tag/v2.14.3 for more details (Kat Marchán) #2822. * src: V8 cpu profiling no longer erroneously shows idle time (Oleksandr Chekhovskyi) #2324. * v8: Lateral upgrade to 4.5.103.33 from 4.5.103.30, contains minor fixes (Ali Ijaz Sheikh) #2870. - This fixes a previously known bug where some computed object shorthand properties did not work correctly (#2507). Refs: #2844 PR-URL: #2889

MylesBorins · 2015-11-16T19:06:40Z

landed in lts-v4.x-staging as ea15d71

indutny added http Issues or PRs related to the http subsystem. land-on-v4.x labels Sep 11, 2015

indutny mentioned this pull request Sep 11, 2015

Node 4 breaks making 16kb or greater requests #2821

Closed

ChALkeR reviewed Sep 11, 2015
View reviewed changes

...

214ed2e

ChALkeR reviewed Sep 11, 2015
View reviewed changes

...

5fe0449

Fishrock123 mentioned this pull request Sep 13, 2015

Release proposal: v4.1.0 #2844

Closed

7 tasks

Fishrock123 closed this Sep 15, 2015

rvagg mentioned this pull request Sep 15, 2015

Release proposal: v3.3.1 #2698

Merged

rvagg mentioned this pull request Sep 22, 2015

Release proposal: v4.1.1 #2995

Merged

jasnell added the lts-landed-on-v4.x label Oct 7, 2015

MylesBorins removed the land-on-v4.x label Nov 16, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http_server: fix resume after socket close #2824

http_server: fix resume after socket close #2824

indutny commented Sep 11, 2015

indutny commented Sep 11, 2015

ChALkeR commented Sep 11, 2015

ChALkeR Sep 11, 2015

indutny Sep 11, 2015

geek commented Sep 11, 2015

ChALkeR Sep 11, 2015

indutny Sep 11, 2015

ChALkeR Sep 11, 2015

ChALkeR Sep 11, 2015

ChALkeR commented Sep 11, 2015

cjihrig commented Sep 11, 2015

Trott commented Sep 11, 2015

silverwind commented Sep 11, 2015

Trott commented Sep 11, 2015

silverwind commented Sep 11, 2015

ChALkeR commented Sep 13, 2015

Fishrock123 commented Sep 14, 2015

indutny commented Sep 14, 2015

Fishrock123 commented Sep 15, 2015

MylesBorins commented Nov 16, 2015

http_server: fix resume after socket close #2824

http_server: fix resume after socket close #2824

Conversation

indutny commented Sep 11, 2015

indutny commented Sep 11, 2015

ChALkeR commented Sep 11, 2015

ChALkeR Sep 11, 2015

Choose a reason for hiding this comment

indutny Sep 11, 2015

Choose a reason for hiding this comment

geek commented Sep 11, 2015

ChALkeR Sep 11, 2015

Choose a reason for hiding this comment

indutny Sep 11, 2015

Choose a reason for hiding this comment

ChALkeR Sep 11, 2015

Choose a reason for hiding this comment

ChALkeR Sep 11, 2015

Choose a reason for hiding this comment

ChALkeR commented Sep 11, 2015

cjihrig commented Sep 11, 2015

Trott commented Sep 11, 2015

silverwind commented Sep 11, 2015

Trott commented Sep 11, 2015

silverwind commented Sep 11, 2015

ChALkeR commented Sep 13, 2015

Fishrock123 commented Sep 14, 2015

indutny commented Sep 14, 2015

Fishrock123 commented Sep 15, 2015

MylesBorins commented Nov 16, 2015