Fix handling of timed out connections kept alive in connection pool under Unix#762
Merged
ras0219-msft merged 5 commits intomicrosoft:masterfrom Aug 3, 2018
Merged
Conversation
Reduce code duplication between ssl_proxy_tunnel::handle_status_line() and the method with the same name in asio_context itself by moving the common handling of connections closed by server into a new function. No real changes, this is a pure refactoring.
When an SSL connection times out due to being closed by server, a different error code is returned, so we need to check for it too in was_closed_by_server(). Without this, losing connection was not detected at all when using HTTPS, resulting in "Failed to read HTTP status line" errors whenever the same http_client was reused after more than the server keep alive timeout of inactivity. See microsoft#592.
Creating a new request when the existing connection being used was closed by the server didn't work correctly because the associated input stream was already exhausted, as its contents had been already "sent" to the server using the now discarded connection, so starting a new request using the same body stream never worked. Fix this by explicitly rewinding the stream to the beginning before using it again. Note that even with this fix using a custom body stream which is not positioned at the beginning initially (or which doesn't support rewinding) still wouldn't work, but at least it fixes the most common use case. See microsoft#592.
…ad_status_line. Avoid increasing public surface with rewind function.
Contributor
|
Agreed. Thanks for the PR and sorry for the long delay! |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This fixes #592 by ensuring that we actually reopen the connection if the existing one, obtained from the connection pool, timed out in the meanwhile.
Note that the existing code already tried to do it, but failed due to 2 bugs in it: first, it didn't detect the timeouts properly for HTTPS connections and, second, it didn't reinitialize the request input stream, meaning that even if the connection was reopened, the correct request wasn't sent over it.
Other improvements remain possible (and desirable), but this PR at least makes things work, albeit not in the most efficient way, whereas they just don't currently, so I think it should already be applied and the rest done later.