-
Notifications
You must be signed in to change notification settings - Fork 30
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
Should mioco catch panics by default? #90
Comments
@jeremyjh @Drakulix @polyfractal , would you like to add your opinion? |
I think by default it should let panic unwind through the scheduler's thread, but there should be a mechanism to catch panic before it crashes the whole scheduler - just as you can in native threads. |
That this library provides panic handling at all is not really idiomatic Rust. A panic implies a critical point of failure, which cannot be recovered in a meaningful way, most of the time related to unsafe code or knowingly invalidating a simple invariant. That recover and propagate is in place is mostly only related to the fact, that it causes undefined behavior (most of the time causing a crash) to unwind into or through foreign (non-rust) code. However I do understand, that in server and production related code, (which is what mioco is somewhat aiming for with evented IO,) you need to protect your whole program against crashing. Even if a panic should never happen. I would say toss all of that panic related logic out of the code (except for the necessary propagate into the scheduler thread) and let the user handle it. This would clean up a good amount of code and gives us the opportunity to do something more useful with the I would even say the current API does encourage you to throw a panic in case of an error, because it is easier to get that information through the Anyway I am also in favor of changing the default behavior. |
Panic has always been intended to be handled at the Task level in Rust; I think that is more idiomatic than using recover statements in the thread that may raise a panic. |
I am not sure, if I understand your last comment @jeremyjh . It seems to be contradictory to your last one. |
Sorry, I'm using old terminology - I should just say thread. We used to have task::try in the standard library, which was the only way to catch a panic - it spawned a new thread and would catch a panic in that thread if it happened - there was no way to recover in the same thread. I think this was removed because it encouraged people to use heavy-weight OS threads just to be able to catch a panic. In mioco, the older model makes more sense to me because cos are lightweight. So all I am asking for is an equivalent of the old task::try, that would spawn a new coroutine and let it contain a panic result and handle it how they wished. See http://smallcultfollowing.com/rust-int-variations/imem-umem/guide-tasks.html for the old behavior. |
Alright that makes sense. You have a point, it is clearly easier to handle the panic through a Also my argument seems to be invalid, I forgot, that coroutines already return a @dpc Do you think it might be possible to make ExitStatus generic over the Results Ok-Variant? Maybe even over the Err Variant. e.g.
|
@jeremyjh : You made a good point. Thanks. I think there's technically no drawback from keeping panic handling, and it seems it has it's own advantages. @Drakulix : It would be possible, but it seems it would require whole mioco instance to be parametrized over Or maybe some |
I think we should just stick to the current implementation then. We should just decide, if we catch panics by default or not. |
My vote would be not catch them by default. On Thursday, February 25, 2016, Victor Brekenfeld notifications@github.com
|
+1 |
All right then. Default is to catch them, there's an option to disable that. I guess we can close. |
Right now mioco catches all panics from coroutines and converts them into
ExitStatus::Panicked
notification.In #85 a config option was added to allow not-catching, and we're wondering if catching panic is justified at all.
The reasoning is:
Looking for opinions about it.
Myself, ATM, I think removing catching panics is a way to go.
The text was updated successfully, but these errors were encountered: