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.

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

Replace http_request_full internals with treq #1058

Merged
7 changes: 4 additions & 3 deletions vumi/tests/fake_connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ def __init__(self, handler):
def endpoint(self):
return self.fake_server.endpoint

def get_agent(self, reactor=None, contextFactory=None):
def get_agent(self, reactor=None, pool=None, contextFactory=None):
"""
Returns an IAgent that makes requests to this fake server.
"""
Expand All @@ -297,6 +297,7 @@ def render_PUT(self, request):


class ProxyAgentWithContext(ProxyAgent):
def __init__(self, endpoint, reactor=None, contextFactory=None):
def __init__(self, endpoint, reactor=None, pool=None, contextFactory=None):
self.contextFactory = contextFactory # To assert on in tests.
super(ProxyAgentWithContext, self).__init__(endpoint, reactor=reactor)
super(ProxyAgentWithContext, self).__init__(
endpoint, reactor=reactor, pool=pool)
4 changes: 2 additions & 2 deletions vumi/tests/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -234,9 +234,9 @@ def test_http_request_with_custom_context_factory(self):

ctxt = WebClientContextFactory()

def stashing_factory(reactor, contextFactory=None):
def stashing_factory(reactor, contextFactory=None, pool=None):
agent = self.fake_http.get_agent(
reactor, contextFactory=contextFactory)
reactor, contextFactory=contextFactory, pool=pool)
agents.append(agent)
return agent

Expand Down
52 changes: 52 additions & 0 deletions vumi/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
from twisted.web.iweb import IBodyProducer
from twisted.web.http import PotentialDataLoss
from twisted.web.resource import Resource
from treq._utils import default_pool, default_reactor
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's a good idea to import from a private module.. These methods could change.

Copy link
Contributor

Choose a reason for hiding this comment

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

And you don't seem to be using default_reactor.

Copy link
Contributor

Choose a reason for hiding this comment

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

when calling default_pool we were sending pool=None and persistence=False. In that case the method will create a new HTTPSConnectionPool. so that is what I did instead.
https://github.com/twisted/treq/blob/master/src/treq/_utils.py#L38-L42

from treq.client import HTTPClient

from vumi.errors import VumiError

Expand Down Expand Up @@ -119,6 +121,56 @@ def connectionLost(self, reason):
def http_request_full(url, data=None, headers={}, method='POST',
timeout=None, data_limit=None, context_factory=None,
agent_class=None, reactor=None):
"""
This is a drop in replacement for the original `http_request_full` method
but it has its internals completely replaced by treq. Treq supports SNI
and our implementation does not for some reason. Also, we do not want
to continue maintaining this because we're favouring treq everywhere
anyway.

"""
agent_class = agent_class or Agent
if reactor is None:
# The import replaces the local variable.
from twisted.internet import reactor
pool = default_pool(reactor, pool=None, persistent=False)
context_factory = context_factory or WebClientContextFactory()
agent = agent_class(reactor, pool=pool, contextFactory=context_factory)
client = HTTPClient(agent)

def handle_response(response):
return SimplishReceiver(response, data_limit).deferred

d = client.request(method, url, headers=headers, data=data)
d.addCallback(handle_response)

if timeout is not None:
cancelling_on_timeout = [False]
Copy link
Contributor

Choose a reason for hiding this comment

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

Reason that this is a list?

Copy link
Contributor

Choose a reason for hiding this comment

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

@smn perhaps you can answer this?

Copy link
Member Author

Choose a reason for hiding this comment

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

the why escapes me, this was many years ago, I got it from the old implementation: https://github.com/praekelt/vumi/pull/1058/files/a22e08f7250ab6570fd96b8469d96090696b4560#diff-832ae36feb20f811191dde6be9132638R195

perhaps @jerith remembers?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this not something to do with that False in python is a simple variable, where as [False] is an object?

>>> False is False
True
>>> [False] is [False]
False```

Copy link
Member

Choose a reason for hiding this comment

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

It's a hack to get a mutable value that can be modified elsewhere:

>>> foo = [1]; print foo, id(foo)
[1] 4359988792
>>> foo[0] = 2; print foo, id(foo)
[2] 4359988792

This lets us pass cancelling_on_timeout through a bunch of other code (which can modify the contents) and then check it later to see if it's been changed. Note that this pattern also requires modifications to be of the form foo[0] = 3 (update the existing list) rather than foo = [3] (replace the list with a new one).

Copy link
Member

Choose a reason for hiding this comment

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

In this particular case, we use cancelling_on_timeout to get information from cancel_on_timeout() (which is scheduled to cancel the request after some time) into the raise_timeout() errback so we can raise a timeout exception instead of a generic cancellation exception or whatever.


def raise_timeout(reason):
if not cancelling_on_timeout[0] or reason.check(HttpTimeoutError):
return reason
return Failure(HttpTimeoutError("Timeout while connecting"))

def cancel_on_timeout():
cancelling_on_timeout[0] = True
d.cancel()

def cancel_timeout(r, delayed_call):
if delayed_call.active():
delayed_call.cancel()
return r

d.addErrback(raise_timeout)
delayed_call = reactor.callLater(timeout, cancel_on_timeout)
d.addCallback(cancel_timeout, delayed_call)

return d


def old_http_request_full(url, data=None, headers={}, method='POST',
timeout=None, data_limit=None, context_factory=None,
agent_class=None, reactor=None):
if reactor is None:
# The import replaces the local variable.
from twisted.internet import reactor
Expand Down