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

use requests library everywhere #34

Merged
merged 1 commit into from
Jan 13, 2020

Conversation

adrpar
Copy link

@adrpar adrpar commented Jul 17, 2019

I have ported the use of http.client in request.py to the requests library.

We have experienced an issue with the tus client behind a proxy, where the client was unable to upload the chunks to the server. Investigating the issue reviled, that the use of http.client was not supporting proxy settings.

Since all the other requests to the server have been done using the requests library, I would like to propose to do the same for request.py and thus get all the proxy support for free.

NOTE: Since handle in TusRequest is a public member, this change might break code that uses the handle directly.

@ifedapoolarewaju
Copy link
Contributor

Hi @adrpar thank you for submitting this PR. Trying to understand the underlying problem a bit here.

Investigating the issue reviled, that the use of http.client was not supporting proxy settings.

So what exactly do we mean by proxy settings here? Is this a problem we could rather solve by adding an option to set proxy URLs? And then passing it down to the http library via set_tunnel?
I imagine something similar is required for the requests library, so how does switching to the requests library fix the issue you are facing?

@adrpar
Copy link
Author

adrpar commented Jul 21, 2019

Hi @ifedapoolarewaju,

sorry for being quite unspecific about the actual problem. The issue is, that I would like to use the python client behind a proxy under linux. The proxy is configured using the HTTP(S)_PROXY env variables. Some hosts are excluded using the NO_PROXY env var.

So the expected behaviour of the library would be, to respect those settings.

The initial POST request initiating the file upload respects those settings, since there the requests library is used. However the subsequent PATCH requests fail to respect the proxy settings, since for that to happen, as you mentioned, you would need to use set_tunnel from http.client.

Since the requests library already does that internally, I think it does not make much sense to reimplement respecting the proxy settings from the environment. But maybe there is a reason on your side to use http.client there, which I am unaware of.

Using requests as in the PR above does fix the issue I am facing behind the proxy though.

@ifedapoolarewaju
Copy link
Contributor

@adrpar thank you for the clarification. I think we can rather check internally if the HTTP(S)_PROXY envs are set and use it accordingly without the need to change the request library. This way the change is more stable and we don't have to worry about any unforseen circumstances. What do you think?

Copy link
Contributor

@ifedapoolarewaju ifedapoolarewaju left a comment

Choose a reason for hiding this comment

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

thank you for the work

@ifedapoolarewaju
Copy link
Contributor

merging in this change since significant changes are now being made to support async uploads

@ifedapoolarewaju ifedapoolarewaju merged commit da76c9c into tus:master Jan 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants