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.

Sign up for GitHub

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

Jump to bottom

Add a generic network exception to be raised #8133

Closed
wants to merge 1 commit into from
Closed

Add a generic network exception to be raised #8133

wants to merge 1 commit into from

Conversation

gutsytechster
Copy link
Contributor

Towards #5380

The other PR may be created for removing the usage of raise_for_status.

@gutsytechster
Copy link
Contributor Author

gutsytechster commented Apr 25, 2020 •
edited
Loading

I want to get reviews on the approach.

@gutsytechster
Copy link
Contributor Author

I am really not sure, where I should put the test? Any hint?

@uranusjr
Copy link
Member

You mean you want to test the network errors are raised and caught correctly? I don’t really think it’s viable TBH. You can probably unit-test the raising part by mocking out some requests internals, but that’s quite complex for a pretty modest return.

@gutsytechster
Copy link
Contributor Author

Yeah, was thinking about that. So, is there something else missing in this PR?

@gutsytechster gutsytechster changed the title WIP: Add a generic network exception to be raised Add a generic network exception to be raised Apr 28, 2020
@deveshks deveshks mentioned this pull request May 15, 2020
Merged
@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label May 24, 2020
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label May 26, 2020
@gutsytechster gutsytechster mentioned this pull request May 30, 2020
Open
@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Jul 16, 2020
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Aug 8, 2020
xavfernandez
xavfernandez reviewed Aug 11, 2020
View reviewed changes
src/pip/_internal/network/utils.py
@@ -95,3 +96,8 @@ def response_chunks(response, chunk_size=CONTENT_CHUNK_SIZE):
if not chunk:
break
yield chunk
except (NewConnectionError, ReadTimeoutError) as exc:
raise NetworkConnectionError(
"Failed to get address for host! Check your network connectivity "
Copy link
Member

Choose a reason for hiding this comment

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

Failed to get address for host! seems weird for a ReadTimeoutError

It definitely makes sense for NewConnectionError in the context of #5380 (comment) but I don't think we should conflate the two exceptions types.

@pradyunsg
Copy link
Member

@ichard26 With your ongoing work around the retrying logic, I wanted to flag this to you -- either to incorporate or close this out given that this is significantly out of date now.

@ichard26
Copy link
Member

Ha, this is an old pull request :)

Catching NewConnectionError and reporting a host address/DNS issue is unfortunately indeed too simplistic. There are other errors that can lead to a NewConnectionError, including a permission error.1 My PR, #12818, handles the exceptions caught by this PR and more. I wish I could simply report "hey, this looks like a DNS issue, check your DNS settings" but determining whether it's a DNS issue would involve parsing the raw error string AFAICT...

Anyhow, this PR isn't necessary. Thanks for bringing this to my attention!

Footnotes

  1. Thinking about it, I should compile a list from various issues on the tracker. A list would be helpful in finalizing my PR. ↩

@ichard26 ichard26 closed this Jul 14, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@xavfernandez xavfernandez xavfernandez left review comments

@uranusjr uranusjr uranusjr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@gutsytechster @uranusjr @BrownTruck @pradyunsg @ichard26 @xavfernandez @pypa-bot