Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 1 addition & 2 deletions stripe/_api_requestor.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
NoReturn,
Unpack,
)
import uuid
from urllib.parse import urlsplit, urlunsplit, parse_qs

# breaking circular dependency
Expand Down Expand Up @@ -580,7 +579,7 @@ def request_headers(

# IKs should be set for all POST requests and v2 delete requests
if method == "post" or (api_mode == "V2" and method == "delete"):
headers.setdefault("Idempotency-Key", str(uuid.uuid4()))
headers.setdefault("Idempotency-Key", os.urandom(16).hex())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Given that we presumably pay the cost to import uuid during import phase not runtime phase, does this change affect the bottom line much?

Also it's a little dumb, but it would be nice if we could re-insert the dashes so it looks like a UUID. It's useful for readability in workbench.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Technically yes, but I was seeing ~1.8s for importing uuid in a CPU-constrained docker, which is just under 20% of the init phase budget. I think it would be better to keep uuid out if we can here, but I can also see about adding the dashes back in.


if method == "post":
if api_mode == "V2":
Expand Down
125 changes: 73 additions & 52 deletions stripe/_http_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,61 +65,11 @@ def _now_ms():


def new_default_http_client(*args: Any, **kwargs: Any) -> "HTTPClient":
"""
This method creates and returns a new HTTPClient based on what libraries are available. It uses the following precedence rules:

1. Urlfetch (this is provided by Google App Engine, so if it's present you probably want it)
2. Requests (popular library, the top priority for all environments outside Google App Engine, but not always present)
3. Pycurl (another library, not always present, not as preferred as Requests but at least it verifies SSL certs)
4. urllib with a warning (basically always present, a reasonable final default)

For performance, it only imports what it's actually going to use. But, it re-calculates every time its called, so probably save its result instead of calling it multiple times.
"""
try:
from google.appengine.api import urlfetch # type: ignore # noqa: F401
except ImportError:
pass
else:
return UrlFetchClient(*args, **kwargs)

try:
import requests # noqa: F401
except ImportError:
pass
else:
return RequestsClient(*args, **kwargs)

try:
import pycurl # type: ignore # noqa: F401
except ImportError:
pass
else:
return PycurlClient(*args, **kwargs)

return UrllibClient(*args, **kwargs)
return _default_sync_client(*args, **kwargs)


def new_http_client_async_fallback(*args: Any, **kwargs: Any) -> "HTTPClient":
"""
Similar to `new_default_http_client` above, this returns a client that can handle async HTTP requests, if available.
"""

try:
import httpx # noqa: F401
import anyio # noqa: F401
except ImportError:
pass
else:
return HTTPXClient(*args, **kwargs)

try:
import aiohttp # noqa: F401
except ImportError:
pass
else:
return AIOHTTPClient(*args, **kwargs)

return NoImportFoundAsyncClient(*args, **kwargs)
return _default_async_client(*args, **kwargs)


class HTTPClient(object):
Expand Down Expand Up @@ -1550,3 +1500,74 @@ async def request_stream_async(

async def close_async(self):
self.raise_async_client_import_error()


# --- Client resolution ---
# Detect available HTTP libraries at module load time so the expensive imports
# (e.g. requests, httpx) happen during Python's init phase rather than when
# StripeClient() is constructed. This matters in environments like AWS Lambda
# where module loading has a generous timeout (10s) but handler invocation
# does not (often 3s).
#
# Sync client precedence:
# 1. Urlfetch (Google App Engine — if present, you probably want it)
# 2. Requests (popular, top priority outside GAE)
# 3. Pycurl (verifies SSL certs, but less preferred than Requests)
# 4. urllib (stdlib fallback, basically always present)
#
# Async client precedence:
# 1. httpx + anyio (both required)
# 2. aiohttp
# 3. NoImportFoundAsyncClient (raises on use)
#
# To add a new client: define the class above, then add it to the appropriate
# cascade below. The resolved class is stored directly — new_default_http_client()
# and new_http_client_async_fallback() just call it.


def _resolve_sync_client():
try:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was digging into this a little because I was worried that reintroducing this import chain to the hot path would undo the problems we were solving in #1427. Those users and the lambda one have sort of opposite goals- a hot reloading django server wants to defer imports as much as possible and lambda wants to frontload as much as it can. So we want to be careful to balance the needs of both users.

I think the httplib checks are cheap enough that putting them back in the hot path isn't the end of the world. But, we might be able to have our cake and eat it too!

There's a performance cost to using try/except as control flow and I believe import misses aren't free either. But, we can check if we could import a module without actually importing it, which is very fast (and i'm not sure why I spaced this approach before):

from importlib.util import find_spec

find_spec('requests') # ModuleSpec(name='requests', loader=<...
find_spec('missing') # None

So I think the fix here is to rewrite this function (and the async one) to check importability of libraries and then return the correct class (to be instantiated as needed).

The downside to this is that inside the clients themselves, they re-import their respective libraries:

class RequestsClient(HTTPClient):
    name = "requests"

    def __init__(
        self,
        ... 
        _lib=None,  # used for internal unit testing
    ):
        ...

        if _lib is None:
            import requests

            _lib = requests

        self.requests = _lib

This is ~free when we've already successfully run a import requests (which was used to determine that we should use this library) but would get called at runtime if we used find_spec to pick a client instead (slowing down the lambda again).

So! I would maybe use find_spec to pick a client (faster for everyone) and then call import <...> to warm up the import cache (slower for django). This is probably close enough to a good balance, but we'll want to listen closely (or understand the performance regression and decide if it's acceptible).

The other option is to control this behavior with an env var, since the two groups want opposite behavior and you can't otherwise change the behavior of import stripe. They're easy to set in lambda, so we could only do the eager import <...> if STRIPE_PYTHON_EAGER_IMPORT is set (which you'd probably only use in lambda).

from google.appengine.api import urlfetch # type: ignore # noqa: F401

return UrlFetchClient
except ImportError:
pass

try:
import requests # noqa: F401

return RequestsClient
except ImportError:
pass

try:
import pycurl # type: ignore # noqa: F401

return PycurlClient
except ImportError:
pass

return UrllibClient


def _resolve_async_client():
try:
import httpx # noqa: F401
import anyio # noqa: F401

return HTTPXClient
except ImportError:
pass

try:
import aiohttp # noqa: F401

return AIOHTTPClient
except ImportError:
pass

return NoImportFoundAsyncClient


_default_sync_client = _resolve_sync_client()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I had a PR comment saying we should pull these because it wasn't obvious what they were doing- I didn't clock that _resolve_sync_client was being called deliberately at import time. So a comment might be good here!

_default_async_client = _resolve_async_client()
8 changes: 4 additions & 4 deletions tests/test_http_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,8 @@ def test_default_httpclient_from_imports(
with patch("builtins.__import__") as mocked_import_fn:
mocked_import_fn.side_effect = mock_import(available_libs)

client = _http_client.new_default_http_client()
assert isinstance(client, expected)
resolved_class = _http_client._resolve_sync_client()
assert resolved_class is expected

@pytest.mark.parametrize(
["available_libs", "expected"],
Expand All @@ -123,8 +123,8 @@ def test_default_async_httpclient_from_imports(
with patch("builtins.__import__") as mocked_import_fn:
mocked_import_fn.side_effect = mock_import(available_libs)

client = _http_client.new_http_client_async_fallback()
assert isinstance(client, expected)
resolved_class = _http_client._resolve_async_client()
assert resolved_class is expected


MakeReqFunc = Callable[[str, str, Dict[str, str], Optional[str]], Any]
Expand Down
Loading