http_server: fix resume after socket close #2824
Conversation
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
|
cc @nodejs/collaborators @ChALkeR |
|
Testing. |
There was a problem hiding this comment.
Imo a new Buffer(16384).fill(0) would be better here. Or with some other value that was causing this.
|
@indutny thanks for the quick turnaround 👍 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Here you go, not sure how relevant it was, though ;)
There was a problem hiding this comment.
The same applies to other tests — some of them fill buffers, some don't. But that is out of scope for now.
|
The fix works for me (when applied on top of v3.3.0). |
|
LGTM pending CI. Kind of funny that the test doesn't actually assert anything. It just passes by not crashing :-) |
|
If that's the case, the |
|
@Trott that could be material for another PR. There's a few tests that have unused local variables. I'll look into that. |
|
@silverwind 👍 That subsequent PR might be able to include the |
|
CI: https://ci.nodejs.org/job/node-test-pull-request/289/ Some ARM machines and FreeBSD failed. CI re-run: https://ci.nodejs.org/job/node-test-pull-request/290/ |
|
CI isn't so happy but it seems unrelated? cc @indutny could we expedite this? :) |
|
@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: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
|
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: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
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>
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
|
landed in lts-v4.x-staging as ea15d71 |
Socket resume may happen on a next tick, and in following scenario:
socket.resume()socket._handle.close()socket._handle = null;The
_resumewill be invoked with empty._handleproperty. There isnothing bad about it, and we should just ignore the
resume/pauseevents in this case.
Same applies to the unconsuming of socket on adding
dataand/orreadableevent listeners.Fix: #2821