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

Add Proxy to WebsocketOptions #431

Merged
merged 3 commits into from Mar 18, 2021
Merged

Add Proxy to WebsocketOptions #431

merged 3 commits into from Mar 18, 2021

Conversation

fulder
Copy link
Contributor

@fulder fulder commented Jun 18, 2020

Add support for custom proxy function for websocket Dialer instead of hardcoding it to http.ProxyFromEnvironment. This hardcode can cause unexpected behavior during use cases where the proxy is changed during runtime as http.ProxyFromEnvironment will only look it up once during the first call.

Also added a warning log if websocket handshake fails due to e.g. invalid status code (other than 101). This response slurped and returned by the Dial function was just ignored and made debugging harder.

@fulder fulder force-pushed the master branch 2 times, most recently from 3730ecf to 23b35d7 Compare June 18, 2020 11:57
Signed-off-by: Michal Sadowski <misad90@gmail.com>
Signed-off-by: Michal Sadowski <misad90@gmail.com>
@fulder fulder force-pushed the master branch 3 times, most recently from 8ea88ba to 39c2d72 Compare June 18, 2020 12:24
Signed-off-by: Michal Sadowski <misad90@gmail.com>
@jeolax
Copy link

jeolax commented Mar 18, 2021

No activity in nine months. This PR would be helpful when proxy isn't set with HTTP_PROXY environment variables. How ca we bring this PR forward?

@MattBrittan MattBrittan merged commit a140ed8 into eclipse:master Mar 18, 2021
@MattBrittan
Copy link
Contributor

Apologies - this came through before I became a committer and I did not notice it when reviewing outstanding commits. Looks good to me.

@fulder
Copy link
Contributor Author

fulder commented Mar 18, 2021

@MattBrittan no worries, thank you for the review and merge 👍

@jeolax
Copy link

jeolax commented Mar 19, 2021

Great! Thanks for the quick response @MattBrittan

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