Replace http_request_full internals with treq #1058
Conversation
|
@smn I just changed the order of things a little |
|
💥 amazing! Thank you :) 🍰 🍰 🍰 👍 |
|
👍 |
vumi/utils.py
Outdated
| from twisted.web.server import Site | ||
| from twisted.web.http_headers import Headers | ||
| 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 |
There was a problem hiding this comment.
I don't think it's a good idea to import from a private module.. These methods could change.
There was a problem hiding this comment.
And you don't seem to be using default_reactor.
There was a problem hiding this comment.
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
| d.addCallback(handle_response) | ||
|
|
||
| if timeout is not None: | ||
| cancelling_on_timeout = [False] |
There was a problem hiding this comment.
@smn perhaps you can answer this?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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```
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
|
@JayH5 ^^ this commit removes usage of |
|
@KaitCrawford I actually read the |
|
Looks like this is definitely fixed in |
This reverts commit d822b65.
No description provided.