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

gix::transport::client::blocking_io::http::reqwest::Remote errors too late #923

Closed
1 task done
Tracked by #949
Jake-Shadle opened this issue Jul 6, 2023 · 3 comments
Closed
1 task done
Tracked by #949
Labels
acknowledged an issue is accepted as shortcoming to be fixed

Comments

@Jake-Shadle
Copy link
Contributor

Jake-Shadle commented Jul 6, 2023

Duplicates

  • I have searched the existing issues

Current behavior 😯

If there is a problem setting up the client, the thread will exit with an error and the channels owned by the thread will be dropped, which will cause panics when the remote tries to be used, such as here.

Expected behavior 🤔

If there is an error setting up the client or other setup issues, I would expect to have a proper error returned from the higher level function (in this case fetch_only) rather than a panic, as it would make it much more clear to users what the root cause of the failure is, as well as not (potentially) bring down their entire application.

This comment also seems misleading.

Steps to reproduce 🕹

  1. Induce a failure in reqwest's clientbuilder, in my case, apparently trust-dns can no longer parse my resolv.conf, there are multiple other ways for it to fail though, unsure what the easiest method to induce failure would be
  2. Do a remote operation eg. gix::prepare_clone().fetch_only()
  3. Observe panic trying to send the request since thread with the client was dropped and there is no request receiver.
@Byron Byron added the acknowledged an issue is accepted as shortcoming to be fixed label Jul 6, 2023
@Byron
Copy link
Member

Byron commented Jul 6, 2023

Thanks for reporting, it's much appreciated!
I wasn't aware, and apparently the comment indicates that it was supposed to work that way (and maybe even does for curl), but doesn't anymore. Maybe it never worked and a test is missing, and I will see to sorting that out soon.

Byron added a commit that referenced this issue Jul 26, 2023
Previously it was possible for reqwest to fail and cause a panic.
Now the actual error will be returned to the original caller.
@Byron
Copy link
Member

Byron commented Jul 26, 2023

This will be fixed with the next release, I will post here when that happens and it should be today or tomorrow.

@Byron Byron mentioned this issue Jul 26, 2023
8 tasks
Byron added a commit that referenced this issue Jul 31, 2023
Previously it was possible for reqwest to fail and cause a panic.
Now the actual error will be returned to the original caller.
@Byron
Copy link
Member

Byron commented Aug 2, 2023

This issue is resolved with the latest release.
Since this backend is rather young and not really used by me, I think it will mostly be user requests that make it better. So please keep them coming if there is a need.

@Byron Byron closed this as completed Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acknowledged an issue is accepted as shortcoming to be fixed
Projects
None yet
Development

No branches or pull requests

2 participants