-
Notifications
You must be signed in to change notification settings - Fork 893
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
Add socks5 support for reqwest-backend #2466
Add socks5 support for reqwest-backend #2466
Conversation
I wonder if there's any way we can add a test to ensure that we don't accidentally lose this capability? |
@kinnison I added a test to check if socks5 feature is enabled. It performs a simple request with non-existent socks5 address from |
I've rebased this (as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point I'm satisfied with the shape of this, but @eidenar needs to double-check and then rebase the series so that we have a nice clean commit series of the change and the test, without the corrections.
In order to ensure that we do not stomp on each other when running tests in the download crate to verify proxy behaviour, introduce a Mutex to guard against this. Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>
I've updated commits and messages to be clear, @kinnison. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, thank you for your contribution.
As discussed in #2409, rustup doesn't use socks5 proxy with reqwest backend. It turned out, that enabling socks feature in Cargo.toml is enough to make it work as intended.
There was no updates from the last person worked on this issue for almost a month, so I decided to pick it up.