-
Notifications
You must be signed in to change notification settings - Fork 824
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
Avoid panics? #6737
Comments
It seems there is potentially an overall project question of when is using a panic desired and what efforts should be made to clean up existing code with this standard? |
Yes I think having clear guidelines on when to panic (basically an uncatchable panic) vs return errors would be helpful I think there is a tension between the decreased usability of APIs that require handling / propagating errors and allowing errors to propagate. In my opinion, if an API returns an error, it is an error that some program might want to catch. If the only errors that can be returned are due to bugs in code (aka internals errors) it is less clear to me an error is appropriate in that case For invalid inputs beyond the control of the calling program (e.g. invalid parquet files) i think returning errors rather than panic is a very reasonable behavior |
Would this be something to discuss on the mailing list (or in this issue) or something else? I understand this can be contentious and I don't have enough knowledge to actually say what is right for the project but would like to see it move forward so happy to help out where I can.
My perspective (which is admittedly not rust centric) comes from operating multi-tenant services. In this case the only cases that we wouldn't want to explicitly catch errors at a higher level is memory corruption (I think there is some gray area for out-of-bounds references). I've seen multiple occurrences of the equivalent in panic in C++, causing wide spread outages due to bugs in the assertion (i.e. the developer did not reason correctly about valid vs invalid state). Using a stack-unwinding panic handler is one way to mitigate but they come with there own issues (e.g. mixed FFI/Rust code). |
Also CC @etseidl who I think had opinions here |
https://doc.rust-lang.org/book/ch09-03-to-panic-or-not-to-panic.html#guidelines-for-error-handling I think is relevant here, panics are a perfectly valid way to handle unexpected or illegal system states, especially when this would have safety implications. Errors should be used where the situation is expected, e.g. due to user input. I would be pretty lukewarm on a load of PRs randomly replacing assertions with errors, this just makes code harder to read and blurs what situations are expected. However, PRs that change panics reachable from public APIs into errors are welcome. It is also worth noting, although mostly irrelevant in practice that handling results is potentially slower on the happy path than panics, as panic unrolling logic is off the hot path
FWIW anyone using the tokio ecosystem is using such a handler |
I recommend we keep the conversation on this ticket as the convention is communication happens on github and thus is is also easier for people to find / discover in the future I have had good luck in the past sending notes to the mailing list with a pointer to ticket when looking to solicit wider feedback, for example: https://lists.apache.org/thread/lqoh9mkss6xlod3380h85pjzowfsysw1 |
Thanks for all the comments! IIUC it seems we have a consensus like this? |
Yes, with a caveat. If a state is reachable upon invalid / corrupted user input then that does not mean the panic should be converted to an error. It is just as likely that validation needs to be inserted into the pipeline earlier. For example, let's say I have some invalid parquet input. Maybe a byte array length is larger than the page length. Maybe this causes us to not read all the items in the array and then we generate some kind of panic like |
Thanks for calling this out. This is better and clearer. |
This is great. Thank you Is anyone willing to try and make a PR to put what appears to be a consensus into the project readme? Perhaps a "Guidelines for |
BTW @pitrou made a good point on the mailing list I will copy here:
|
This is a follow-up for the discussion here. In short, the parquet implementation may panic upon invalid inputs. IIUC ideally we should avoid panics as much as possible, especially those can be converted to error results with small efforts.
The text was updated successfully, but these errors were encountered: