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.

Sign up for GitHub

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

Jump to bottom

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.

@not-implemented
http: fix missing close event on aborted response
79f7f34 
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.
@not-implemented not-implemented mentioned this pull request Apr 8, 2015
Closed
@Fishrock123 Fishrock123 added the http Issues or PRs related to the http subsystem. label Apr 8, 2015
@Fishrock123
Copy link
Contributor

cc @indutny?

brendanashworth
brendanashworth reviewed Apr 12, 2015
View reviewed changes
test/parallel/test-http-response-close-event-race.js
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
@not-implemented @indutny
test: add test for missing close/finish event
b6b37a5 
See next commit for the actual fix.

PR-URL: nodejs#1373
Reviewed-By: Fedor Indutny <fedor@indutny.com>
indutny added a commit to indutny/io.js that referenced this pull request Apr 13, 2015
@indutny
http_server: prefinish vs finish
2811b0c 
Do not detach the socket from the response until all data is actually
sent to the other side.

See: nodejs#1373
@indutny indutny mentioned this pull request Apr 13, 2015
Closed
@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
@not-implemented @indutny
test: add test for missing close/finish event
7170741 
See next commit for the actual fix.

PR-URL: nodejs#1373
Reviewed-By: Fedor Indutny <fedor@indutny.com>
indutny added a commit to indutny/io.js that referenced this pull request Apr 15, 2015
@indutny
http_server: prefinish vs finish
6b03dd8 
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
@not-implemented @indutny
test: add test for missing close/finish event
159577c 
See next commit for the actual fix.

PR-URL: nodejs#1373
Reviewed-By: Fedor Indutny <fedor@indutny.com>
indutny added a commit to indutny/io.js that referenced this pull request Jun 1, 2015
@indutny
http_server: prefinish vs finish
854a071 
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
@indutny @Fishrock123
http_server: prefinish vs finish
6020d2a 
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 <christopher.s.dickinson@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
indutny added a commit that referenced this pull request Jun 17, 2015
@indutny @chrisdickinson
http_server: prefinish vs finish
646d67b 
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 <christopher.s.dickinson@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
indutny pushed a commit that referenced this pull request Jul 4, 2015
@not-implemented @indutny
test: add test for missing close/finish event
9436a86 
See next commit for the actual fix.

PR-URL: #1373
Reviewed-By: Fedor Indutny <fedor@indutny.com>
indutny pushed a commit that referenced this pull request Jul 4, 2015
@not-implemented @indutny
test: add test for missing close/finish event
7387297 
See next commit for the actual fix.

PR-URL: #1373
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@Fishrock123 Fishrock123 mentioned this pull request Jul 6, 2015
Merged
indutny added a commit that referenced this pull request Jul 22, 2015
@indutny @rvagg
http_server: prefinish vs finish
ce1acd5 
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 <christopher.s.dickinson@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
indutny added a commit that referenced this pull request Jul 24, 2015
@indutny @rvagg
http_server: prefinish vs finish
9b1341c 
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 <christopher.s.dickinson@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
indutny added a commit that referenced this pull request Jul 30, 2015
@indutny @rvagg
http_server: prefinish vs finish
577acc4 
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 <christopher.s.dickinson@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
indutny added a commit that referenced this pull request Aug 1, 2015
@indutny @rvagg
http_server: prefinish vs finish
da5ba35 
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 <christopher.s.dickinson@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
indutny added a commit that referenced this pull request Aug 3, 2015
@indutny @rvagg
http_server: prefinish vs finish
d1974e2 
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 <christopher.s.dickinson@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
indutny added a commit that referenced this pull request Aug 4, 2015
@indutny @rvagg
http_server: prefinish vs finish
9f3946a 
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 <christopher.s.dickinson@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
indutny added a commit that referenced this pull request Aug 4, 2015
@indutny @rvagg
http_server: prefinish vs finish
11e4249 
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 <christopher.s.dickinson@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@JanCantCode JanCantCode mentioned this pull request Aug 23, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@not-implemented @Fishrock123 @indutny @brendanashworth