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

pyfetch: Do not raise an error on status code >= 400 #3986

Merged
merged 3 commits into from Jul 12, 2023

Conversation

owenlamont
Copy link
Contributor

@owenlamont owenlamont commented Jul 8, 2023

Description

The previous logic was raising an OSError if a response returned a status code at 400 or greater but there are valid reasons to retrieve the bodies from such responses as they often contain additional information about the error that can be useful to the client - see issue 3974.

Checklists

string returns as it is valid to retrieve the bodies
from these responses.
@hoodmane
Copy link
Member

hoodmane commented Jul 8, 2023

Thanks @owenlamont. I wonder if we might want to just change raise_if_failed to raise only if bodyUsed is true? Since we should still be raising an error in that case.

@ryanking13
Copy link
Member

Thanks @owenlamont!

I wonder if we might want to just change raise_if_failed to raise only if bodyUsed is true? Since we should still be raising an error in that case.

+1 with this.

400 status check from _raise_if_failed, and added test for
new behavior
@owenlamont
Copy link
Contributor Author

Hope this looks better, I haven't been able to get a local build working to run tests myself - trying to follow the Linux Conda setup instructions on WSL 2 but running make gave me errors about too many open files and I tried to join the Gitter channel to ask about that, but Gitter locks up for me trying to open the pyodide community room from the link in the documentation (I had no trouble joining other rooms).

Copy link
Member

@ryanking13 ryanking13 left a comment

Choose a reason for hiding this comment

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

Thanks, @owenlamont! I have a minor comment about the test, otherwise looks good to me.

trying to follow the Linux Conda setup instructions on WSL 2 but running make gave me errors about too many open files

I'm not very sure about the too many open files issue but WSL2 has some issues with the file system, and it has some trouble especially when using the Windows file system in WSL2. So personally I don't recommend using WSL2 when building Pyodide.

src/tests/test_pyodide_http.py Outdated Show resolved Hide resolved
@hoodmane hoodmane merged commit 822276d into pyodide:main Jul 12, 2023
38 checks passed
@hoodmane hoodmane changed the title Removed raise if failed check from the json and string response returns pyfetch: Do not raise an error on status code >= 400 Jul 12, 2023
@ryanking13
Copy link
Member

Thanks again @owenlamont!

@owenlamont
Copy link
Contributor Author

Thanks for all the support @ryanking13 I appreciate all the work the Pyodide devs have done.

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

3 participants