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: fix missing close event on aborted response #1373

Closed
wants to merge 1 commit into from
Closed

http: fix missing close event on aborted response #1373

wants to merge 1 commit into from

Conversation

not-implemented
Copy link
Contributor

When sending a response after client aborted the connection in the same tick when the socket is already destroyed but the socket-close event is still not delivered, you did not get either a response-finish or a response-close event.

This fix of the race-condition needs an additional flag "_finishOrCloseEmitted" in OutgoingMessage.

I did not find any easier solution - I tried to fix this either with an additional "close" or an additional "finish" with some timing-tricks without an additional flag. I had no chance without breaking anything else.

Not all of the new checks of "_finishOrCloseEmitted" are currently really needed - they are just for completeness.

When sending a response after client aborted the connection in the same
tick when the socket is already destroyed but the socket-close event is
still not delivered, you did not get either a response-finish or a
response-close event.

This fix of the race-condition needs an additional flag
"_finishOrCloseEmitted" in OutgoingMessage.
@Fishrock123 Fishrock123 added the http Issues or PRs related to the http subsystem. label Apr 8, 2015
@Fishrock123
Copy link
Contributor

cc @indutny?

var testTickCount = 3;

var server = http.createServer(function (req, res) {
console.log('server: request');
Copy link
Contributor

Choose a reason for hiding this comment

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

usually, passing tests should be silent; could you remove the expected console.logs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course I could remove them - I kept with the other tests which also contain console.logs (output is hidden anyway in normal test run).
I thought, especially race conditions are much more understandable with some debugging output - also for future refactoring/debugging.

Copy link
Member

Choose a reason for hiding this comment

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

Please keep them for now :) I've a bit delayed this, but going to review it next week!

indutny pushed a commit to indutny/io.js that referenced this pull request Apr 13, 2015
See next commit for the actual fix.

PR-URL: nodejs#1373
Reviewed-By: Fedor Indutny <[email protected]>
indutny added a commit to indutny/io.js that referenced this pull request Apr 13, 2015
Do not detach the socket from the response until all data is actually
sent to the other side.

See: nodejs#1373
@indutny
Copy link
Member

indutny commented Apr 13, 2015

@not-implemented PTAL at #1411 . I have integrated your test, but decided to choose a bit different way to fix the issue.

@indutny
Copy link
Member

indutny commented Apr 13, 2015

Thank you for discovering this!

@not-implemented
Copy link
Contributor Author

Ah very cool ... I didn't have the guts to delay the WHOLE finish-handling - only parts of it ;-)
Thanks!

@indutny
Copy link
Member

indutny commented Apr 13, 2015

Thank you!

indutny pushed a commit to indutny/io.js that referenced this pull request Apr 15, 2015
See next commit for the actual fix.

PR-URL: nodejs#1373
Reviewed-By: Fedor Indutny <[email protected]>
indutny added a commit to indutny/io.js that referenced this pull request Apr 15, 2015
Do not detach the socket from the response until all data is actually
sent to the other side.

See: nodejs#1373
indutny pushed a commit to indutny/io.js that referenced this pull request Jun 1, 2015
See next commit for the actual fix.

PR-URL: nodejs#1373
Reviewed-By: Fedor Indutny <[email protected]>
indutny added a commit to indutny/io.js that referenced this pull request Jun 1, 2015
Do not detach the socket from the response until all data is actually
sent to the other side.

See: nodejs#1373
indutny added a commit that referenced this pull request Jun 5, 2015
Do not detach the socket from the response until all data is actually
sent to the other side.

See: #1373
PR-URL: #1411
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
indutny added a commit that referenced this pull request Jun 17, 2015
Do not detach the socket from the response until all data is actually
sent to the other side.

See: #1373
PR-URL: #1411
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
indutny pushed a commit that referenced this pull request Jul 4, 2015
See next commit for the actual fix.

PR-URL: #1373
Reviewed-By: Fedor Indutny <[email protected]>
indutny pushed a commit that referenced this pull request Jul 4, 2015
See next commit for the actual fix.

PR-URL: #1373
Reviewed-By: Fedor Indutny <[email protected]>
indutny added a commit that referenced this pull request Jul 22, 2015
Do not detach the socket from the response until all data is actually
sent to the other side.

See: #1373
PR-URL: #1411
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
indutny added a commit that referenced this pull request Jul 24, 2015
Do not detach the socket from the response until all data is actually
sent to the other side.

See: #1373
PR-URL: #1411
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
indutny added a commit that referenced this pull request Jul 30, 2015
Do not detach the socket from the response until all data is actually
sent to the other side.

See: #1373
PR-URL: #1411
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
indutny added a commit that referenced this pull request Aug 1, 2015
Do not detach the socket from the response until all data is actually
sent to the other side.

See: #1373
PR-URL: #1411
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
indutny added a commit that referenced this pull request Aug 3, 2015
Do not detach the socket from the response until all data is actually
sent to the other side.

See: #1373
PR-URL: #1411
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
indutny added a commit that referenced this pull request Aug 4, 2015
Do not detach the socket from the response until all data is actually
sent to the other side.

See: #1373
PR-URL: #1411
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
indutny added a commit that referenced this pull request Aug 4, 2015
Do not detach the socket from the response until all data is actually
sent to the other side.

See: #1373
PR-URL: #1411
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
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.

4 participants