Skip to content

Remove unnecessary route delay for Windows 7 and newer. #772

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bernerdad
Copy link

This PR considers removing a 5 second delay on Windows when setting up the tunnel. We have been using this change for 3+ years in the Windscribe desktop app on Windows 10/11, with no adverse impact.

Thank you for your contribution

You are welcome to open PR, but they are used for discussion only. All
patches must eventually go to the openvpn-devel mailing list for review:

Please send your patch using git-send-email. For example to send your latest commit to the list:

$ git send-email [email protected] HEAD~1

For details, see these Wiki articles:

@schwabe
Copy link
Contributor

schwabe commented Jun 25, 2025

Could you please explain how you come to the conclusion that this is no longer necessary and how you tested that?

@@ -3243,7 +3243,7 @@ options_postprocess_mutate_invariant(struct options *options)
/* delay may only be necessary when we perform DHCP handshake */
const bool dhcp = (options->tuntap_options.ip_win32_type == IPW32_SET_DHCP_MASQ)
|| (options->tuntap_options.ip_win32_type == IPW32_SET_ADAPTIVE);
if ((options->mode == MODE_POINT_TO_POINT) && dhcp)
if ((options->mode == MODE_POINT_TO_POINT) && dhcp && (win32_version_info() <= WIN_VISTA))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't support anything older than Vista anymore and probably not even Vista at this point. So you really would need to remove the whole block instead, instead of basically making is dead code.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't aware Win7 was the minimum supported OS now. I have removed the dead code. With respect to concluding this code is no longer necessary; our Windscribe desktop application has been running with this change in production since late 2021, with ~25000 daily active OpenVPN connections. Prior to making this change, we observed a substantial delay when initializing the tunnel on Windows. Removing this 5s delay, and another one that has already been fixed by OpenVPN, resolved this delay.

Remove dead code since Windows Vista and older are no longer supported.
@schwabe
Copy link
Contributor

schwabe commented Jun 27, 2025

Yes. But your windscribe tunnel is only one very specific tunnel configuration. So you basically have only tested it in one very specific configuration that removing the delay works. Without really knowing what the specific problem was that this delay address, it is a hard to judge it if it is safe to remove nowadays.

Also if such a behavioural change is itroduced, it should be mentioned in the Changes.rst

@cron2
Copy link
Contributor

cron2 commented Jun 27, 2025

Yes. But your windscribe tunnel is only one very specific tunnel configuration. So you basically have only tested it in one very specific configuration that removing the delay works. Without really knowing what the specific problem was that this delay address, it is a hard to judge it if it is safe to remove nowadays.

The overall point is somewhat moot, as this is in a code path that nobody really gets to use anymore in new OpenVPN installs - it's using TAP+DHCP, while really everything should use win-dco and ipapi (via iservice) these days... which does not suffer from automatic route-delay settings for DHCP.

Removing code paths that made sense on long-forgotten Windows parts is actually a good thing :-) - having a bit more testing would be good, though.

Also if such a behavioural change is itroduced, it should be mentioned in the Changes.rst

Indeed. But since half the regular contributors send stuff without Changes.rst updates these days, it's not something that would hold up the patch if acceptable otherwise.

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.

3 participants