fix: don't break out of run_connection_attempts with futures left to try - #8458
fix: don't break out of run_connection_attempts with futures left to try#8458link2xt wants to merge 1 commit into
futures left to try#8458Conversation
…o try If we ran out of running connection attempts, but there are still futures not added to the connection attempt set, we should not break out with the list of errors yet, but move another future into the attempt set even if the delay has not expired yet. Otherwise it is possible to resolve the hostname into two IPs, try one that fails immediately, then break out of the loop without trying the second one and only report the error for the first one. It is still not nice that we may start another connection attempt right before the next delay expires, so we may start two connections attempts almost simultaneously, but it is not critical.
| } | ||
| } | ||
| None => { | ||
| // Out of connection attempts. |
There was a problem hiding this comment.
This comment here was wrong, it was possible that we have empty connection_attempt_set, but non-empty futures.
|
I have not seen any logs that look like having this problem. I was looking at the log line So i have looked at the code for no good reason, but the code does not look correct. |
| /// and runs them until one of them succeeds | ||
| /// or all of them fail. | ||
| /// | ||
| /// If all connection attempts fail, returns the first error. |
There was a problem hiding this comment.
The comment is outdated, need to update as we return all errors.
|
The function should be unit-tested with some futures that sleep and return success/errors. |
|
Replaced with #8459 |
1 participant
If we ran out of running connection attempts,
but there are still futures not added to the connection attempt set, we should not break out with the list of errors yet, but move another future into the attempt set
even if the delay has not expired yet.
Otherwise it is possible to resolve the hostname into two IPs, try one that fails immediately,
then break out of the loop without trying the second one and only report the error for the first one.
It is still not nice that we may start another connection attempt right before the next delay expires, so we may start two connections attempts almost simultaneously, but it is not critical.