Fix requests processing loop to honor KeepAliveTimeout #46
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Admin is allowed to configure a KeepAliveTimeout:
The maximum time in seconds waiting for a new requests before a
connection will be closed.
If the value is set to -1, there is no timeout.
The processing loop calls keepalive_request() to implement the wait;
'keepalive_request' can return either due to a inactivity timeout, or
upon connection close/error, or due to a new request pending.
Alas, the return code isn't checked, and the code always continues to
beginning of loop, invoking a new 'process_request()' call.
The problem is that in a *genuine* timeout event (no further exchanges
from the icap client on the connection), continuing to process_request
will cause an *additional* blocking wait:
process_request()
do_request()
parse_header()
ci_read_icap_header(req, h, TIMEOUT) # defaults to 300s
wait_for_data(req->connection, timeout, ci_wait_for_read)
which means the KeepAliveTimeout value isn't really honored, and the
worker thread will stall for an additional timeout (either until new
request arrives or until client closed the connection, or timeout).
Fix, by testing keepalive_request() return code: if it is not positive
(i.e. timeout or error), just break out of the requests processing loop,
which will close the connection.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Admin is allowed to configure a KeepAliveTimeout:
The maximum time in seconds waiting for a new requests before a
connection will be closed.
If the value is set to -1, there is no timeout.
The processing loop calls keepalive_request() to implement the wait; 'keepalive_request' can return either due to a inactivity timeout, or upon connection close/error, or due to a new request pending.
Alas, the return code isn't checked, and the code always continues to beginning of loop, invoking a new 'process_request()' call.
The problem is that in a genuine timeout event (no further exchanges from the icap client on the connection), continuing to process_request will cause an additional blocking wait:
process_request()
do_request()
parse_header()
ci_read_icap_header(req, h, TIMEOUT) # defaults to 300s
wait_for_data(req->connection, timeout, ci_wait_for_read)
which means the KeepAliveTimeout value isn't really honored, and the worker thread will stall for an additional timeout (either until new request arrives or until client closed the connection, or timeout).
Fix, by testing keepalive_request() return code: if it is not positive (i.e. timeout or error), just break out of the requests processing loop, which will close the connection.