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

Implement server_poll_timeout for socks instead of hard-coded 5 sec #411

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

5andr0
Copy link

@5andr0 5andr0 commented Sep 15, 2023

People have been struggling to run openvpn over slow proxies.
There was a bug #328 submitted 10 years ago.
To fix this, the server-poll-timeout config value was introduced, but has only been implemented for http proxies.
For socks the timeouts are still hard-coded to 5 sec.
That's why people are still having problems, see issue #267

I submitted a patch to the mailing list, but since this was my first time using a mailing list over git, I failed to provide details in the mail.

With this patch, establishing a socks connection uses the timeout set by server-poll-timeout (default: 120s).
I used the same logic like it was implemented for http proxies, which includes the socks handshake for the timeout.

So here's a PR with more details, in hope that it gets reviewed and implemented soon.

@cron2
Copy link
Contributor

cron2 commented Sep 15, 2023

A commit needs to have a good description on what is changed and why - there is no difference between "PR" and "on the list" in this. Your commit has a summary, but the description that is here in the PR ("the same logic that was implemented for http proxies") needs to go into the actual commit message - when people look at code change some years from now, github might long be gone (so whatever is written in the PR comments is long history), but the actual git commit message will be still there.

Also, this PR has a merge commit, which it shouldn't have (so always rebase on master before creating a patch).

... but do not despair, we do have the patch on the list, and with the description from the PR, I can construct a more verbose commit message. I just want a review, preferrably from @schwabe who understands all this timeout stuff (having rewritten much of it a few years ago)...

Connections over SOCKS are timed out after hard-coded 5 sec,
which causes problems when using slow proxies like Tor.

With this patch, establishing a SOCKS connection uses the timeout
set by server-poll-timeout (default: 120s).
It uses the same logic that was implemented for HTTP proxies,
which includes the SOCKS handshake for the timeout.

Github: fixes OpenVPN#267
@5andr0
Copy link
Author

5andr0 commented Sep 16, 2023

A commit needs to have a good description on what is changed and why - there is no difference between "PR" and "on the list" in this. Your commit has a summary, but the description that is here in the PR ("the same logic that was implemented for http proxies") needs to go into the actual commit message - when people look at code change some years from now, github might long be gone (so whatever is written in the PR comments is long history), but the actual git commit message will be still there.

I highly appreciate you taking the time to explain how it's done properly. From now on I'll always write multi-line commit messages.

Also, this PR has a merge commit, which it shouldn't have (so always rebase on master before creating a patch).

Rebased and amended a detailed commit message 👍🏻

... but do not despair, we do have the patch on the list, and with the description from the PR, I can construct a more verbose commit message. I just want a review, preferrably from @schwabe who understands all this timeout stuff (having rewritten much of it a few years ago)...

Thanks for taking over!

@ValdikSS
Copy link
Contributor

ValdikSS commented May 4, 2024

@cron2
LGTM, tested on 2.5.8

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