Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Recreate HTTPSConnection after errors #567

Open
wants to merge 2 commits into
base: v3-v2021-02-25
Choose a base branch
from
Open

Recreate HTTPSConnection after errors #567

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8e018af
Recreate HTTPSConnection after errors
dgilmanAIDENTIFIED Jun 16, 2022
250825f
Merge branch 'v3-v2021-02-25' into fix-httplib-state-machine
dgilmanAIDENTIFIED Jun 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 16 additions & 4 deletions recurly/base_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,10 @@
import socket
from base64 import b64encode
import json
from . import resources
from .resource import Resource, Empty
from .request import Request
from .response import Response
from recurly import USER_AGENT, DEFAULT_REQUEST_TIMEOUT, ApiError, NetworkError
from pydoc import locate
import urllib.parse
from datetime import datetime
from enum import Enum
Expand All @@ -30,7 +28,6 @@ def request_converter(value):
class BaseClient:
def __init__(self, api_key, timeout=None, **options):
self.__api_key = api_key
actual_timeout = timeout if timeout is not None else DEFAULT_REQUEST_TIMEOUT
api_host = HOST

if "region" in options:
Expand All @@ -42,8 +39,16 @@ def __init__(self, api_key, timeout=None, **options):

api_host = API_HOSTS[options["region"]]

self.__actual_timeout = (
timeout if timeout is not None else DEFAULT_REQUEST_TIMEOUT
)
self.__api_host = api_host
self._create_conn()
self.__needs_reset = False

def _create_conn(self):
self.__conn = http.client.HTTPSConnection(
api_host, PORT, timeout=actual_timeout
self.__api_host, PORT, timeout=self.__actual_timeout
)

def _make_request(self, method, path, body, **options):
Expand All @@ -66,9 +71,16 @@ def _make_request(self, method, path, body, **options):
if "params" in options:
path += "?" + self._url_encode(options["params"])

# Every __conn must have getresponse() called on it successfully
# or it must be thrown away
if self.__needs_reset:
self._create_conn()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems simple enough!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


self.__needs_reset = True
self.__conn.request(method, path, body, headers=headers)
request = Request(method, path, body)
resp = Response(self.__conn.getresponse(), request)
self.__needs_reset = False

if resp.status >= 400:
if resp.body:
Expand Down