ntop / n2n

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

Add close window support for Windows #1111

Closed Legend-Master closed 1 year ago

Legend-Master commented 1 year ago

Add cleanup support for closing the terminal/shell window on Windows

codecov-commenter commented 1 year ago

Codecov Report

Patch and project coverage have no change.

Comparison is base (6ecc201) 22.62% compared to head (ad0e805) 22.62%.

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## dev #1111 +/- ## ======================================= Coverage 22.62% 22.62% ======================================= Files 42 42 Lines 8601 8601 ======================================= Hits 1946 1946 Misses 6655 6655 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

hamishcoleman commented 1 year ago

I've been trying to understand the logic here.

I'm guessing, but I think what is happening is - when the signal handler returns, windows immediately kills the process and all its threads. So, if we signal the main loop to exit and never return from the signal handler, that gives the main loop time to do any cleanup needed. So, what kills the sleep() thread in the signal handler?

Legend-Master commented 1 year ago

Without it, when you close the terminal/shell window, it won't have the time to clean up which will cause authentication error if you try to reconnect

Blocking this infinitely is fine since when our main thread returns, it will shut down anyway

I'm guessing, but I think what is happening is - when the signal handler returns, windows immediately kills the process and all its threads.

Yes, exactly

I found Go is doing this when I was searching, and there's a comment explained how it works (it also mentioned a library called libuv which is used by Node.js): https://github.com/golang/go/issues/41884#issuecomment-706695923

hamishcoleman commented 1 year ago

Thanks for expanding on the explanation

hamishcoleman commented 1 year ago

Is it expected that it takes 5 minutes between Ctrl-C and the process actually exiting?

Legend-Master commented 1 year ago

Is it expected that it takes 5 minutes between Ctrl-C and the process actually exiting?

No, I created an issue about this at #1087 2 months ago

And if you mean 5 seconds on hitting the x from the Windows console window, then that's the time when Windows decides to kill the program no matter if it's finished or not

hamishcoleman commented 1 year ago

I wondered if it was related to the sleep(infinite).

I dont run long running tasks in console windows, so I would never close those windows from hitting the x.

These select timeouts make sense on everything other than windows. Lets keep that other ticket open and I will see if I can look at it some time.

Legend-Master commented 1 year ago

I wondered if it was related to the sleep(infinite).

I don't think it runs on ctrl c

These select timeouts make sense on everything other than windows.

I will take a look on select, if I remembered correctly the shutdown is all good when I ran the supernode on linux, but not on windows

hamishcoleman commented 1 year ago

The select() timeout should be 10 seconds, but I am seeing anything from 3 minutes to 9 minutes!

The messages clearly show that it is going through the term_handler() codepath, but I dont have a debugger installed on my windows test VMs, so I have not traced the exact place it is hanging - so it could be anywhere.

I don't think I have seen these kind of delays on a linux systems - I would remember.