-
Notifications
You must be signed in to change notification settings - Fork 32
Add a timeout to shutdown #369
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
Conversation
lbialy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A test would be nice. It doesn't have to actually deadlock, you could just sleep, capture IEx on user's main fiber and continue sleeping until some deadline (longer than the timeout provided in settings, eg timeout 100ms, user's fiber sleep deadline 200ms)
|
Hm I'm not sure if this would work, as when the timeout passes, we're not interrupting anything (well we're interrupting the waiting for the main thread to finish), but that main thread will continue working, and will prevent the scope from exiting. That's different from an exit process, which will just kill everything when the shutdown hook finishes. |
|
That's why I proposed that instead of a timeout we just race the interrupt with sleep(timeout)+Runtime.halt. The way you've implemented it can do the same if you call Runtime.halt after the warning to stderr is printed. The situation is that it is indeed a failure situation (in cleanup protocol) if that timeout is crossed so we can just kill the jvm there and then. We then avoid the problem of |
|
Hm well I'm not convinced to do |
|
Ok, you can write a test, maybe not perfect, but we are testing some parts |
|
So once the shutdown hook finishes, the deadlock resolves and the main fiber joins correctly? |
|
the main fiber, after interrupting, sleeps for 200ms, which is longer than the timeout (100ms). So we aren't simulating a deadlock, rather a cleanup that's longer |
|
Sure, I'm not talking about the test, the test is fine. I was thinking about the |
|
Ah sorry ;) Yes of course, I tested that manually |
Partially resolves #368