Skip to content
This repository has been archived by the owner on Apr 14, 2022. It is now read-only.

Ensure import urllib3 can succeed even if trio and twisted aren't installed #42

Merged
Merged
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
68 changes: 68 additions & 0 deletions test/test_backends.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
import sys

import pytest

import urllib3
from urllib3.backends import Backend
from urllib3._backends._loader import normalize_backend, load_backend


requires_async_pool_manager = pytest.mark.skipif(
not hasattr(urllib3, "AsyncPoolManager"),
reason="async backends require AsyncPoolManager",
)


requires_sync_pool_manager = pytest.mark.skipif(
hasattr(urllib3, "AsyncPoolManager"),
reason="sync backends cannot be used with AsyncPoolManager",
)


class TestNormalizeBackend(object):
"""
Assert that we fail correctly if we attempt to use an unknown or incompatible backend.
"""
def test_unknown(self):
with pytest.raises(ValueError) as excinfo:
normalize_backend("_unknown")

assert 'unknown backend specifier _unknown' == str(excinfo.value)

@requires_sync_pool_manager
def test_sync(self):
assert normalize_backend(Backend("sync")) == Backend("sync")
assert normalize_backend("sync") == Backend("sync")
assert normalize_backend(None) == Backend("sync")

with pytest.raises(ValueError) as excinfo:
normalize_backend(Backend("trio"))

assert 'trio backend requires urllib3 to be built with async support' == str(excinfo.value)

@requires_async_pool_manager
def test_async(self):
assert normalize_backend(Backend("trio")) == Backend("trio")
assert normalize_backend("twisted") == Backend("twisted")

with pytest.raises(ValueError) as excinfo:
normalize_backend(Backend("sync"))

assert 'sync backend requires urllib3 to be built without async support' == str(excinfo.value)

from twisted.internet import reactor
assert normalize_backend(Backend("twisted", reactor=reactor)) == Backend("twisted", reactor=reactor)


class TestLoadBackend(object):
"""
Assert that we can load a normalized backend
"""
@requires_sync_pool_manager()
def test_sync(self):
load_backend(normalize_backend("sync"))

@requires_async_pool_manager()
def test_async(self):
from twisted.internet import reactor
load_backend(Backend("twisted", reactor=reactor))
1 change: 0 additions & 1 deletion test/test_no_ssl.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,5 @@ def import_ssl():

self.assertRaises(ImportError, import_ssl)

@pytest.mark.xfail
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this test now passes. hooray!

def test_import_urllib3(self):
import urllib3 # noqa: F401
7 changes: 7 additions & 0 deletions test/with_dummyserver/test_no_ssl.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,17 @@
from dummyserver.testcase import (
HTTPDummyServerTestCase, HTTPSDummyServerTestCase)

import pytest
import urllib3


class TestHTTPWithoutSSL(HTTPDummyServerTestCase, TestWithoutSSL):

@pytest.mark.skip(reason=(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this test is cursed. boo.

"TestWithoutSSL mutates sys.modules."
"This breaks the backend loading code which imports modules at runtime."
"See discussion at https://github.com/python-trio/urllib3/pull/42"
))
def test_simple(self):
pool = urllib3.HTTPConnectionPool(self.host, self.port)
self.addCleanup(pool.close)
Expand Down
5 changes: 2 additions & 3 deletions urllib3/_async/connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@
)
from urllib3.packages import six
from ..util import ssl_ as ssl_util
from .._backends import SyncBackend
from .._backends._common import LoopAbort
from .._backends._loader import load_backend

try:
import ssl
Expand Down Expand Up @@ -311,8 +311,7 @@ def __init__(self, host, port, backend=None,
source_address=None, tunnel_host=None, tunnel_port=None,
tunnel_headers=None):
self.is_verified = False

self._backend = backend or SyncBackend()
self._backend = backend or load_backend("sync")
Copy link
Contributor Author

@oliland oliland May 3, 2018

Choose a reason for hiding this comment

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

Question: if this function constantly calls from .sync_backend import SyncBackend - is it constantly re-running the import machinery whenever we create a connection?

If so, we should special case SyncBackend to skip the import machinery in the interests of making the common case more efficient.

Alternatively, we should ensure that users have always initialized a backend by this point (which is how I plan for async backends to work), and just leave the or case for unit tests.

Copy link
Member

Choose a reason for hiding this comment

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

Re-importing an already-imported module is pretty cheap, because python immediately notices that it already has the loaded module cached and uses that. So it's like a few dict lookups.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, also: what we actually want the logic to be here (I think) is:

  • If this is the sync version of the code, then the backend must be None or "sync" or Backend("sync"). Anything else is an error.

  • If this is the async version of the code, then the backend must be specified, and must be not the sync backend.

And... we kind of want to do this check inside all of the classes that take backend arguments.

So I suggest adding a normalize_backend function that takes a user backend argument, and returns a valid Backend or else an error.

The trickiest bit is detecting whether we are in sync mode or async mode. Eventually we should add a nice way to do this to the code generation script, but for now one can do:

async def f():
    return None

obj = f()
if obj is None:
    SYNC_MODE = True
else:
    # prevent unawaited coroutine warning
    obj.close()
    SYNC_MODE = False

self._host = host
self._port = port
self._socket_options = (
Expand Down
9 changes: 0 additions & 9 deletions urllib3/_backends/__init__.py
Original file line number Diff line number Diff line change
@@ -1,9 +0,0 @@
from urllib3.packages import six
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really see the point in this file any more, so I truncated it. I guess it would make sense to put load_backend in here, and have that be the only interface to this module, but that feels like making a decision.

Additionally, I'm not sure why a private module (_backends) would expose a public interface (via __all__)?

Copy link
Member

Choose a reason for hiding this comment

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

If I did that then probably my logic was, this is a private API from the user point of view (they shouldn't mention _backend at all), and an opaque module from the point of view of the rest of the internal code (they can use the stuff in _backend.__all__, but shouldn't go digging deeper).

This is a pretty fine distinction and not particularly important.

from .sync_backend import SyncBackend

__all__ = ['SyncBackend']

if six.PY3:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW, this doesn't seem correct - we should be targeting 3.5+, not 3+ (although maybe twisted is a special case)

from .trio_backend import TrioBackend
from .twisted_backend import TwistedBackend
__all__ += ['TrioBackend', 'TwistedBackend']
102 changes: 102 additions & 0 deletions urllib3/_backends/_loader.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
import sys

from ..backends import Backend


class Loader:

def __init__(self, name, loader, is_async):
self.name = name
self.loader = loader
self.is_async = is_async

def __call__(self, *args, **kwargs):
return self.loader(kwargs)


def load_sync_backend(kwargs):
from .sync_backend import SyncBackend
return SyncBackend(**kwargs)


def load_trio_backend(kwargs):
from .trio_backend import TrioBackend
return TrioBackend(**kwargs)


def load_twisted_backend(kwargs):
from .twisted_backend import TwistedBackend
return TwistedBackend(**kwargs)


def backend_directory():
"""
We defer any heavy duty imports until the last minute.
"""
loaders = [
Loader(
name="sync",
loader=load_sync_backend,
is_async=False,
),
Loader(
name="trio",
loader=load_trio_backend,
is_async=True,
),
Loader(
name="twisted",
loader=load_twisted_backend,
is_async=True,
),
]
return {
loader.name: loader for loader in loaders
}


def async_supported():
"""
Tests if the async keyword is supported.
"""
async def f():
"""
Functions with an `async` prefix return a coroutine.
This is removed by the bleaching code, which will change this function to return None.
"""
return None

obj = f()
if obj is None:
return False
else:
obj.close() # prevent unawaited coroutine warning
return True


def normalize_backend(backend):
if backend is None:
backend = Backend(name="sync") # sync backend is the default
elif not isinstance(backend, Backend):
backend = Backend(name=backend)

loaders_by_name = backend_directory()
if backend.name not in loaders_by_name:
raise ValueError("unknown backend specifier {}".format(backend.name))

loader = loaders_by_name[backend.name]

is_async_supported = async_supported()
if is_async_supported and not loader.is_async:
raise ValueError("{} backend requires urllib3 to be built without async support".format(loader.name))

if not is_async_supported and loader.is_async:
raise ValueError("{} backend requires urllib3 to be built with async support".format(loader.name))

return backend


def load_backend(backend):
loaders_by_name = backend_directory()
loader = loaders_by_name[backend.name]
return loader(backend.kwargs)
6 changes: 3 additions & 3 deletions urllib3/_backends/twisted_backend.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import socket
import OpenSSL.crypto
from twisted.internet import protocol, ssl
from twisted.internet import protocol, reactor as default_reactor, ssl
from twisted.internet.interfaces import IHandshakeListener
from twisted.internet.endpoints import HostnameEndpoint, connectProtocol
from twisted.internet.defer import (
Expand All @@ -15,8 +15,8 @@


class TwistedBackend:
def __init__(self, reactor):
self._reactor = reactor
def __init__(self, reactor=None):
Copy link
Contributor Author

@oliland oliland May 3, 2018

Choose a reason for hiding this comment

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

This change attempts to ensure that all of our backends can be initialized without any arguments, which would be nice.

self._reactor = reactor or default_reactor

async def connect(self, host, port, connect_timeout,
source_address=None, socket_options=None):
Expand Down
12 changes: 12 additions & 0 deletions urllib3/backends.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
class Backend:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a user (or downstream repository) interface.

"""
Specifies the desired backend and any arguments passed to its constructor.

Projects that use urllib3 can subclass this interface to expose it to users.
"""
def __init__(self, name, **kwargs):
self.name = name
self.kwargs = kwargs

def __eq__(self, other):
return self.name == other.name and self.kwargs == other.kwargs