-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
Node 4 breaks making 16kb or greater requests #2821
Comments
First bad commit is 1bc4468 (#2355). |
Btw, the description is a bit incorrect: |
Strange, I can't get it to actually throw. It just hangs for me. On which OS are we seeing this? |
@evanlucas Mine — Arch Linux x86_64, both self-built and official binaries. |
There is definitely a bug, confirmed on my mac. The fix is easy, I just want to be sure that all edge cases are covered. |
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
Fix is here: #2824 |
It (the original issue, not the fix that was just added) fails on payload lengths exactly from
|
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>
Should be fixed 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>
Fixed in 7ec0491, to be released soon. |
@Fishrock123 thanks for helping to get this in for 4.0.1! |
Rename test-dgram-regress-4496 to test-dgram-typeerror-buffer to test-dgram-send-invalid-msg-type Rename test-http-regr-nodejsgh-2821 to test-http-request-large-payload Rename test-child-process-fork-regr-nodejsgh-2847 to test-child-process-fork-closed-channel-segfault Rename test-http-pipeline-regr-2639 to test-http-pipeline-serverresponse-assertionerror Rename test-http-pipeline-regr-3332 to test-http-pipeline-requests-connection-leak Rename test-http-pipeline-regr-3508 to test-http-pipeline-socket-parser-typeerror Refs: nodejs#19105 Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure
Related to this PR: #2355
In iojs 3.2.0 the following code works, in iojs 3.3.0 and node 4 the following code throws an error.
Error:
If you change the payload size to just under 16kb it works:
The text was updated successfully, but these errors were encountered: