-
Notifications
You must be signed in to change notification settings - Fork 131
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
KaitCrawford
merged 11 commits into
develop
from
feature/SEED-927-vumi-http-request-full-fails-sni
May 9, 2017
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
62ff4f8
replace with treq
smn b527f69
use the context factory
smn d35e97f
fix custom context factory tests
smn 12fbbda
fix test_http_request_full_timeout_after_connect
smn 82015b2
Add callback to request before handling timeout
f3d826e
Merge branch 'develop' into feature/SEED-927-vumi-http-request-full-f…
927669d
Replace vulnerable HTTPS policy for verifying certificates
a22e08f
Don't set context_factory if one isn't provided
7bc6185
Explicity create connection pool rather than importing a private module
d822b65
Remove use of vulnerable HTTPS policy from sandbox
89d0a42
Revert "Remove use of vulnerable HTTPS policy from sandbox"
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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:
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 formfoo[0] = 3
(update the existing list) rather thanfoo = [3]
(replace the list with a new one).There was a problem hiding this comment.
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 fromcancel_on_timeout()
(which is scheduled to cancel the request after some time) into theraise_timeout()
errback so we can raise a timeout exception instead of a generic cancellation exception or whatever.