[monitoringlib] Set ConnectionResetError as retryable #1310
[monitoringlib] Set ConnectionResetError as retryable #1310BenjaminPelletier merged 1 commit intointeruss:mainfrom
Conversation
| # ('Connection aborted.', RemoteDisconnected('Remote end closed connection without response')) | ||
| # ...and this may be retryable | ||
| retryable = True | ||
| elif "ConnectionResetError" in str(e): |
There was a problem hiding this comment.
While I'm sure this will work fine most of the time practically, can we make this more robust by checking the actual type rather than looking in the string representation? And, perhaps checking the message for "Connection reset by peer" if we always see that in the errors we think are valid to retry?
There was a problem hiding this comment.
I did the same as the existing check on RemoteDisconnected, because I did assume that was done like this because the type was not differentiable, but it's seems to be possible to move up in the exception tree to do a proper class check.
A ConnectionResetError correspond to a '104/Connection reset by peer' (ECONNRESET == 104), so we don't need to check the errno / message if I understand it correctly.
I let you check the improved version. I had to do exact type comparison to avoid getting false positives if we get exceptions extending ConnectionResetError (Like RemoteDisconnected...), but we may also just want to check for subclasses of ConnectionResetErrors.
Follow #1298 to avoid conflicts on the same code area.
Fix #1306 by making ConnectionResetError retryable.
Not tested, as the change is simple and creating an environment with random connection reset is complex.