Handle aborted connection#95
Conversation
withinboredom
force-pushed
the
handle-connection-aborts
branch
from
f0e95bd to
292545f
Compare
|
|
||
| size_t wrote = go_ub_write(ctx->current_request ? ctx->current_request : ctx->main_request, (char *) str, str_length, &failed); | ||
|
|
||
| if(failed) { |
There was a problem hiding this comment.
| if(failed) { | |
| if (failed) { |
(we need to check and fix the CS in the CI btw)
There was a problem hiding this comment.
Is there a C-style checker? I'll admit I only recently noticed the space between if & (.
|
Things to test when compared to fastcgi/apache:
Due to the very nature of networks, I expect several race conditions need to be accounted for in unit tests (i.e., we can't detect a socket is closed until we write to it and get a reply that it is closed.) |
withinboredom
force-pushed
the
handle-connection-aborts
branch
from
1eb124b to
3e93e6e
Compare
dunglas
force-pushed
the
feat/implement-flush
branch
from
cd4974e to
c43dd38
Compare
withinboredom
force-pushed
the
handle-connection-aborts
branch
from
81c7339 to
7eb3e8d
Compare
| return true | ||
| } | ||
|
|
||
| flusher.Flush() |
There was a problem hiding this comment.
Since Flush() on a closed connection seems to be a noop, I'm inclined to remove the previous if-statement and just return the result of clientHasClosed(). What are your thoughts on that?
There was a problem hiding this comment.
It's definitely a micro-optim, but doing this in this order prevents a useless call to Flush() event if it's a noop. I don't have a strong opinion on this TBH.
There was a problem hiding this comment.
True. I don't have strong feelings either, so I'll leave as-is. My optimization thought was mostly to reduce the branching, but it's probably better to leave that sort of optimization for a compiler and just describe what is correct.
dunglas
left a comment
dunglas
left a comment
There was a problem hiding this comment.
LGTM. Do you think we can prevent the sleep() in the tests?
I've been mulling it over. It appears that it all resolves in less than a single millisecond, even on the GitHub runners, so if it becomes an issue, we bump it up to a couple of ms and/or commit upstream to zaptest to get some kind of channel on the log output so we can block until logs are output. There is certainly a race condition here, but I can't think of any way to currently get around it, though I'm open to ideas. |
|
Thank you! |
2 participants
This PR to #90 handles detecting a closed connection and setting the connection state to aborted both during a flush and during a write to output.
Since we were not previously setting the connection state to aborted, this is a breaking change in that by default, PHP stops working once the connection is aborted unless the user calls
ignore_user_abort(true);or theignore_user_abortsetting inphp.ini.