Recreate HTTPSConnection after errors #567
Conversation
|
This would also probably fix #378 |
|
We've been using this in production for a year now successfully so I hope this can be merged as it fixes what was for us a frequent issue. I am going to leave this PR open in hopes the Recurly team can take it over and merge the fix. However, I am no longer going to maintain it as we are no longer Recurly customers. |
|
@bhelx - will this be implemented - I just ran into a similar problem. |
| # Every __conn must have getresponse() called on it successfully | ||
| # or it must be thrown away | ||
| if self.__needs_reset: | ||
| self._create_conn() |
There was a problem hiding this comment.
It seems like to recreate the entire connection every call might be wasteful for the problem. However i'm not sure what the result of this is. Does it also recreate the underlying connection or just the connection object? Is there a more precise way to detect when the connection needs to be reset?
There was a problem hiding this comment.
needs_reset is toggled back to False after a successful Response() (see below). So this path is only ever triggered when you had an exception that prevented the toggling.
There was a problem hiding this comment.
Another way of looking at it: if you were creating a new Recurly object for each API call you avoided this bug but you made a lot of redundant connection objects and Recurly instances. If you didn't make a new Recurly object each time your Recurly instance would occasionally break when the unhandled exception was raised below. But with this change you're reusing a connection object if there is no exception raised and re-creating one when there is an exception / it's in the broken state.
|
@davidemerritt i haven't worked for Recurly since 2020. So pinging @douglasmiller for this issue. |
|
Just a note: resetting the connection every call seems kind of extreme. Perhaps you can just catch the exception But there could be some deeper problem worth looking into. |
|
thanks @bhelx! @douglasmiller - I would expect that the |
This is a fix to #556
If a network error happens when
self.__conn.requestis called the underlying HTTPSConnection is left in a bad state. If you're trying to retry errors the next request raises aCannotSendRequestwhen you get to the nextself.__conn.requestcall.This PR keeps track of successful
self.__conn.getrequest()calls and resets the connection object if a previous request was not successful.