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

Less wrapping of network errors #4064

Merged
merged 1 commit into from Mar 4, 2021
Merged

Conversation

deivid-rodriguez
Copy link
Member

I don't think it's useful to wrap network errors like this, but I might be missing something. I believe they tend to hide errors from the users. If something in particular should be better worded, I believe the builtin exception should be changed instead.

What was the end-user or developer problem that led to this PR?

I tried the new output after #4061 by disconnecting from the internet and running bundle install on an application. The results are:

Fetching source index from https://rubygems.org/

Retrying fetcher due to error (2/4): Bundler::HTTPError Could not fetch specs from https://rubygems.org/ due to underlying error <no such name (https://rubygems.org/specs.4.8.gz)>

Retrying fetcher due to error (3/4): Bundler::HTTPError Could not fetch specs from https://rubygems.org/ due to underlying error <no such name (https://rubygems.org/specs.4.8.gz)>

Retrying fetcher due to error (4/4): Bundler::HTTPError Could not fetch specs from https://rubygems.org/ due to underlying error <no such name (https://rubygems.org/specs.4.8.gz)>

Could not fetch specs from https://rubygems.org/ due to underlying error <no such name (https://rubygems.org/specs.4.8.gz)>

I think the message could be more informative including the underlying network error class, for example.

What is your fix for the problem, implemented in this PR?

Stop wrapping dns errors with a custom special exception to get a more wordy message. With this PR:

Fetching source index from https://rubygems.org/

Retrying fetcher due to error (2/4): Bundler::HTTPError Could not fetch specs from https://rubygems.org/ due to underlying error <SocketError: Failed to open TCP connection to rubygems.org:443 (getaddrinfo: Name or service not known) (https://rubygems.org/specs.4.8.gz)>

Retrying fetcher due to error (3/4): Bundler::HTTPError Could not fetch specs from https://rubygems.org/ due to underlying error <SocketError: Failed to open TCP connection to rubygems.org:443 (getaddrinfo: Name or service not known) (https://rubygems.org/specs.4.8.gz)>

Retrying fetcher due to error (4/4): Bundler::HTTPError Could not fetch specs from https://rubygems.org/ due to underlying error <SocketError: Failed to open TCP connection to rubygems.org:443 (getaddrinfo: Name or service not known) (https://rubygems.org/specs.4.8.gz)>

Could not fetch specs from https://rubygems.org/ due to underlying error <SocketError: Failed to open TCP connection to rubygems.org:443 (getaddrinfo: Name or service not known) (https://rubygems.org/specs.4.8.gz)>

Make sure he following tasks are checked

@bronzdoc
Copy link
Member

Looks good, would you add some tests for this?

I don't think it's useful to wrap network errors like this, but I might
be missing something. I believe they tend to hide errors from the users.
If something in particular should be better worded, I believe the
builtin exception should be changed instead.
@deivid-rodriguez
Copy link
Member Author

Almost 4 months later, I added some tests 😝.

@deivid-rodriguez deivid-rodriguez merged commit be32c40 into master Mar 4, 2021
@deivid-rodriguez deivid-rodriguez deleted the even_better_http_errors branch March 4, 2021 12:52
deivid-rodriguez added a commit that referenced this pull request Mar 8, 2021
Less wrapping of network errors

(cherry picked from commit be32c40)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants