-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[ENH] catch panics in task operators #2450
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
1fefdb6
to
8590025
Compare
.await; | ||
|
||
// Re-panic so the message handler can catch it | ||
panic!( |
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 little weird but I think this makes sense, since the panic technically happened within a component
@@ -46,3 +46,9 @@ pub(crate) trait ChromaError: Error + Send { | |||
} | |||
|
|||
impl Error for Box<dyn ChromaError> {} | |||
|
|||
impl ChromaError for Box<dyn ChromaError> { |
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.
thank you :)
@@ -17,16 +22,45 @@ where | |||
async fn run(&self, input: &I) -> Result<O, Self::Error>; | |||
} | |||
|
|||
#[derive(Debug, Error)] | |||
pub(super) enum TaskError<Err> { |
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.
may want to constrain Err to ChromaError here
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.
adding that bound results in needing to add a lot of spurious typings, so imo it's ok to leave it unbounded and introduce it if there's an issue later
anywhere that calls .boxed()
or .code()
already effectively constrains it
Sg
Hammad Bashir
EECS | Cal
…On Tue, Jul 9, 2024 at 5:19 PM Max Isom ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In rust/worker/src/execution/operator.rs
<#2450 (comment)>:
> @@ -17,16 +22,45 @@ where
async fn run(&self, input: &I) -> Result<O, Self::Error>;
}
+#[derive(Debug, Error)]
+pub(super) enum TaskError<Err> {
adding that bound results in needing to add a lot of spurious typings, so
imo it's ok to leave it unbounded and introduce it if there's an issue later
anywhere that calls .boxed() or .code() already effectively constrains it
—
Reply to this email directly, view it on GitHub
<#2450 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABKW32KJ6JRCWOKQB6I55TTZLR42LAVCNFSM6AAAAABKKMG6DOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCNRXG4ZDMNJZGQ>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
This can't share code with the handler panic-catching-impl because the task has its own reply channel that the handler knows nothing about.