Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

http_server: fix resume after socket close #2824

Closed
wants to merge 3 commits into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Sep 11, 2015

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

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 indutny added http Issues or PRs related to the http subsystem. land-on-v4.x labels Sep 11, 2015
@indutny
Copy link
Member Author

indutny commented Sep 11, 2015

cc @nodejs/collaborators @ChALkeR

@ChALkeR
Copy link
Member

ChALkeR commented Sep 11, 2015

Testing.
cc @geek

port: common.PORT
});

const payload = new Array(1640).join('0123456789');
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, fixed.

@geek
Copy link
Member

geek commented Sep 11, 2015

@indutny thanks for the quick turnaround 👍

port: common.PORT
});

const payload = new Buffer(16390);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@indutny Thanks!

@ChALkeR
Copy link
Member

ChALkeR commented Sep 11, 2015

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

@cjihrig
Copy link
Contributor

cjihrig commented Sep 11, 2015

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

@Trott
Copy link
Member

Trott commented Sep 11, 2015

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

@silverwind
Copy link
Contributor

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

@Trott
Copy link
Member

Trott commented Sep 11, 2015

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

@silverwind
Copy link
Contributor

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

@Fishrock123 Fishrock123 mentioned this pull request Sep 13, 2015
7 tasks
@ChALkeR
Copy link
Member

ChALkeR commented Sep 13, 2015

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
Copy link
Member

CI isn't so happy but it seems unrelated?

cc @indutny could we expedite this? :)

@indutny
Copy link
Member Author

indutny commented Sep 14, 2015

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

indutny added a commit that referenced this pull request Sep 15, 2015
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: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@Fishrock123
Copy link
Member

Landed as requested in 7ec0491

indutny added a commit that referenced this pull request Sep 15, 2015
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: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
indutny added a commit that referenced this pull request Sep 15, 2015
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: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@rvagg rvagg mentioned this pull request Sep 15, 2015
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Sep 17, 2015
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
Fishrock123 added a commit that referenced this pull request Sep 17, 2015
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
@rvagg rvagg mentioned this pull request Sep 22, 2015
@MylesBorins
Copy link
Member

landed in lts-v4.x-staging as ea15d71

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Node 4 breaks making 16kb or greater requests
9 participants