http: handle idle timeout when connection is not yet established #42756
Conversation
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
|
|
||
| void onIdleTimeout() { | ||
| host_->cluster().trafficStats()->upstream_cx_idle_timeout_.inc(); | ||
| close(); |
There was a problem hiding this comment.
what's happening if we close a connection which is not established? I wonder what is happening in such case. I believe the close is just ignored
There was a problem hiding this comment.
That is the bug we are facing #42753 and this fix for bug.
close on a connection not even established is resulting in local connection failure .
|
/assign @botengyao (Senior Maintainer Review) |
|
/retest |
|
@botengyao not urgent. PTAL when you get chance |
botengyao
left a comment
botengyao
left a comment
There was a problem hiding this comment.
Thanks for tackling this down! Left a high level comment.
/wait
source/common/http/codec_client.h
Outdated
| void onIdleTimeout() { | ||
| host_->cluster().trafficStats()->upstream_cx_idle_timeout_.inc(); | ||
| close(); | ||
| if (connected_) { |
There was a problem hiding this comment.
This guard is good, but I think a better approach is don't let the idle timer fire at all while connecting. I’d prefer guard in enableIdleTimer() and we only call enableIdldTimer() when the connection is connected, and there are 2 places, rather than the existing approach during the active client construction. Once the upstream connection is connected, the upstream client will be moved to the ready_clients_ to accept streams, so the idle timeout will be cleared / disabled at that time.
There was a problem hiding this comment.
I was thinking about that but took this approach as it is a low risk fix. I wasn't sure of changing idle timeout logic (moving away from constructor). Any way, changed as per your comments. PTAL.
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
|
/retest |
|
@botengyao This needs your feedback. |
6 participants
Commit Message: handle idle timeout when connection is not yet established
Additional Description: Handles a case where idle timeout is triggered before a connection is established
Risk Level: Low
Testing: Updated
Docs Changes: N/A
Release Notes: Added
Fixes #42753