search

ntop / n2n

Peer-to-peer VPN
GNU General Public License v3.0
6.22k stars 935 forks source link

Fix edge slow shutdown on Windows #1121

Closed Legend-Master closed 6 months ago

Legend-Master commented 1 year ago

Fix #1087 partially

codecov-commenter commented 1 year ago

Codecov Report

Attention: 79 lines in your changes are missing coverage. Please review.

Comparison is base (fff50a7) 21.58% compared to head (1ca14fc) 21.47%.

Files Patch % Lines
src/edge_utils.c 0.00% 79 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## dev #1121 +/- ## ========================================== - Coverage 21.58% 21.47% -0.12% ========================================== Files 46 46 Lines 8991 9038 +47 ========================================== Hits 1941 1941 - Misses 7050 7097 +47 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

hamishcoleman commented 1 year ago

This is quite an invasive change. I would like to see less ifdefs and specific platform code in the core - not more.

Perhaps we can consider other options?

Legend-Master commented 1 year ago

This is quite an invasive change. I would like to see less ifdefs and specific platform code in the core - not more.

I would like too, but I don't think there's anyway to interrupt select

Perhaps we can consider other options?

I would say just make pulling timeout shorter on Windows, like 1-3 seconds (I'm currently doing this for my own use)

hamishcoleman commented 1 year ago

I see you have changed the winsock include - is there a reason for that? is there any side effects? should we consider doing that as a separate update to the timeout PR?

Legend-Master commented 1 year ago

I see you have changed the winsock include - is there a reason for that? is there any side effects? should we consider doing that as a separate update to the timeout PR?

WSAWaitForMultipleEvents only exits in winsock2, and I heard mixing them is problematic on https://stackoverflow.com/a/9168850

winsock2 should be fully compatible in the current code base, there're only a few thing like WSACancelBlockingCall were removed (I tried this function to cancel select but can't make it work even with winsock.h)

If we choose to use the reduced timeout method, then this change is not needed

hamishcoleman commented 1 year ago

Yeah, I've read that SO article a while ago - but I'm not sure why this codebase has the older header.

Unfortunately the windows source code environment seems to be quite hostile to the usual programming techniques that people have been using for more decades than windows has existed :-( -- if the new header is a 100% replacement for the old header (excepting functions you have chosen to drop support for), then just replace it, dont hand the future a maintenance problem!

Legend-Master commented 1 year ago

So what do you think, reduce timeout or WSAWaitForMultipleEvents or anything else?

I think the reduced timeout is fine after using it for a few months

If we choose the timeout one, then I think we can make another PR to unify winsock.h to winsock2.h (or reverse)

hamishcoleman commented 6 months ago

I believe all these slow windows shutdown concerns are fixed in the recently released fork n3n. So, if you could to try that out and and raise a ticket there if it is still happening, we would be happy to investigate it further as part of that new project.

Legend-Master commented 6 months ago

Just tried it, worked well with connect and then ctrl-c

Closing this pull request then