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

ValueError when calling requests.get on windows systems. #6104

Closed
lsrzj opened this issue Apr 6, 2022 · 14 comments
Closed

ValueError when calling requests.get on windows systems. #6104

lsrzj opened this issue Apr 6, 2022 · 14 comments

Comments

@lsrzj
Copy link

lsrzj commented Apr 6, 2022

Using windows I'm getting a ValueError in utils.py file telling that it's failing to parse to int ''. It's because the registry query is returning empty string instead of '0' or '1'.

Expected Result

The request be successful

Actual Result

File ...\site-packages\requests\utils.py, line 68, in proxy_bypass_registry proxyEnable = int(winreg.QueryValueEx(internetSettings,
ValueError: invalid literal for int() with base 10: ''

Reproduction Steps

import requests
url = 'https://api.github.com/events'
r = requests.get(url)

Here is the StackOverflow question of more people complaining about this issue and my answer with a solution for the problem

https://stackoverflow.com/a/71770718/2726538

@nateprewitt
Copy link
Member

nateprewitt commented Apr 6, 2022

Hi @lsrzj, can you please provide the debugging info from the issue template? Looking at the change that added the int requirement, I'm not sure why this hasn't been raised before. I don't see anything in the documentation suggesting this would need to be an integer. The second value in the tuple is explicitly and int though.

We may need a change here to support whatever the variance is in the registry, but that will need a closer look.

@lsrzj
Copy link
Author

lsrzj commented Apr 6, 2022

Hi @lsrzj, can you please provide the debugging info from the issue template? Looking at the change that added the int requirement, I'm not sure why this hasn't been raised before. I don't see anything in the documentation suggesting this would need to be an integer. The second value in the tuple is explicitly and int though.

We may need a change here to support whatever the variance is in the registry, but that will need a closer look.

Problem is that I'm on a remote machine with restricted internet access and I can't copy and paste from my Amazon Workspaces, so I gave the maximum detail I could with prints of the code and showing where the error is. By the way the remote machine is windows server 2016 datacenter version 1607. So I don't know if there's something different when querying Windows 10 or Windows 11 that makes this error not be raised. Fact is, more people is complaining as the sent StackOverflow link shows. And the int() convertion is already in the code, I don't know why it's necessary, the only thing I did was maintaining the original logic to convert it, as it's already converting, but, before converting, I'm checking if it's getting an empty string to not get a ValueError

@nateprewitt
Copy link
Member

@lsrzj Would you at least be able to provide the Python version and Requests version by hand? It looks like this is just a registry issue though. The value is documented by Microsoft as being either 0 or 1, so the fact it returns '' seems like an issue with the registry.

Either way, we should just be catching ValueError here and bypassing the function if the value returned isn't discernible.

@lsrzj
Copy link
Author

lsrzj commented Apr 6, 2022

@lsrzj Would you at least be able to provide the Python version and Requests version by hand? It looks like this is just a registry issue though. The value is documented by Microsoft as being either 0 or 1, so the fact it returns '' seems like an issue with the registry.

Either way, we should just be catching ValueError here and bypassing the function if the value returned isn't discernible.

Python version is 3.9.10 and requests version is 2.27.1. About the solution catching is really better.

@nateprewitt nateprewitt added this to the 2.28.0 milestone Apr 7, 2022
@xororist
Copy link

Hello @nateprewitt! I'm new to contributing to projects I would like to try to fix this issue, can you assign me this task?
Thanks!

@lsrzj
Copy link
Author

lsrzj commented Apr 11, 2022

One thing that may make it difficult to make people upgrade requests library is that pip, itself, depends on requests library, so if windows users tries to update the requests library from pip, they'll get this error and pip will break it's execution. It will be necessary to put a documentation on how to workaround the problem before calling pip, and then, upgrade for the fixed version.

@nateprewitt
Copy link
Member

nateprewitt commented Apr 11, 2022

Thanks for voluteering, @hugo-cachon! I've assigned you, please let us know if you have questions.

@lsrzj I'm not sure I understand your concern. The current error has been possible in Requests for several years and is only triggered by bad registry entries. The proposed fix is to catch that, and disables use of registry proxy configurations when an unknown entry exists. That can only prevent an error in pip, not add one. We also vendor Requests in pip, so your installed version is entirely unrelated to its usage. If your pip installation is broken by the error, it has been broken since at least pip 10 (2017), and won't be fixed until we merge a fix downstream.

@lsrzj
Copy link
Author

lsrzj commented Apr 11, 2022

Thanks for voluteering, @hugo-cachon! I've assigned you, please let us know if you have questions.

@lsrzj I'm not sure I understand your concern. The current error has been possible in Requests for several years and is only triggered by bad registry entries. The proposed fix is to catch that, and disables use of registry proxy configurations when an unknown entry exists. That can only prevent an error in pip, not add one. We also vendor Requests in pip, so your installed version is entirely unrelated to its usage. If your pip installation is broken by the error, it has been broken since at least pip 10 (2017), and won't be fixed until we merge a fix downstream.

My concern is about people that has this problem trying to download the requests fixed version from pip and not being able to do so, because, pip, itself, will have problem to download the fixed requests module version, because of this error. So, whoever has this problem, will have to, firstly, make a manual correction on the module to make it work, and, then, make the pip call to upgrade the requests module version.

@nateprewitt
Copy link
Member

My concern is about people that has this problem trying to download the requests fixed version from pip and not being able to do so, because, pip, itself, will have problem to download the fixed requests module version, because of this error. So, whoever has this problem, will have to, firstly, make a manual correction on the module to make it work, and, then, make the pip call to upgrade the requests module version.

That's incorrect, you'll need to download the new version of pip and the problem will be resolved. It's entirely unrelated to the version of Requests you've installed with pip as I stated in the last comment.

@xororist
Copy link

Thanks @nateprewitt for putting me on this one! I'm thinking about trying to recreate the issue in a virtual machine using the same windows environnement/ python & Requests version.
Is it okay to process the issue that way?
If that's so, I'll come back with further informations once I've made some tests on it

@nateprewitt
Copy link
Member

@hugo-cachon, if it would help you build confidence in your fix, that's a fine approach. Note you'll need to modify your registry entry though as I don't believe this is related to default Windows.

Given we already know the outcome of the failure though, it may be sufficient to catch the error where I noted above, and write a mocked test to ensure proxy selection works as expected.

@xororist
Copy link

Hello @nateprewitt !
Coming with updates and questions on this issue, I was wondering if catching the ValueError and just dealing with it using pass is sufficient or is it better to treat it with setting 0 or 1 as values if this error is raised and depending on what the value of proxyEnable is?

This question comes along the mock test where I was thinking testing the value returned by this method.

Thanks for your time and explanations!

@nateprewitt
Copy link
Member

Hi @hugo-cachon,

I don't believe we want to pass but rather return False. The exact change proposed is changing this line to:

except (OSError, ValueError):

We don't have enough information to makes an assumption around 0 or 1, so we'll bypass the override.

For tests, it would be reasonable to test 0, 1 and "" as return values for winreg.QueryValueEx.

@nateprewitt
Copy link
Member

We've gone ahead and created a PR for this in #6149 to unblock the upcoming 2.28.0 release. Now that it's merged, we'll resolve this and the fix can be expected in the next release. Thanks again @lsrzj for reporting this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants