Skip to content
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

Support try-catch for both invoke and run #65

Merged
merged 5 commits into from
Sep 1, 2024

Conversation

KiKoS0
Copy link
Collaborator

@KiKoS0 KiKoS0 commented Aug 29, 2024

Summary

If an exception is thrown in the invoke or run function, it will now
be caught and returned as a StepError exception. This will allow the
error to be propagated to the caller and handled appropriately.

Transforming the errors into a StepError exception is following the
SDK spec:
https://github.com/inngest/inngest/blob/main/docs/SDK_SPEC.md#522-memoizing-a-step
JS SDK behavior:
https://github.com/inngest/inngest-js/blob/4f91d9c302592ecc2228914469dd057ae148005b/packages/inngest/src/components/execution/v1.ts#L437-L443
Docs:
https://www.inngest.com/docs/features/inngest-functions/error-retries/inngest-errors#step-errors

StepError is a RuntimeException which means that it can be used in Java code without needing to change the interface of Inngest.execute to introduce checked exceptions or break existing step.run & step.invoke calls by requiring strict handling of the exception.

Usage

Invoke

try {
    step.invoke<Map<String, Any>>(
        "send-email",
        "ktor-dev",
        "Email",
        mapOf("email" to "[email protected]"),
        null,
    )
} catch (e: StepError) {
    // Handle error
}

Run

try {
    step.run("send-email") {
        // Run the step
    }
} catch (e: StepError) {
    // Handle error
}

Changes:

  • Create a StepError exception that users can catch to handler errors.
  • Add try/catch support for step.run.
  • Add try/catch support for step.invoke.
  • Implement example Kotlin functions demonstrating the usage.
  • Implement integration tests for them.

Checklist

  • Update documentation
  • Added unit/integration tests

Related

@KiKoS0 KiKoS0 requested a review from albertchae August 29, 2024 21:08
@KiKoS0 KiKoS0 self-assigned this Aug 29, 2024
Copy link

linear bot commented Aug 29, 2024

@KiKoS0 KiKoS0 marked this pull request as draft August 29, 2024 21:14
@KiKoS0 KiKoS0 force-pushed the riadh/inn-3330-support-try-catch branch from 24fa4a9 to d36545d Compare August 30, 2024 18:26
@KiKoS0 KiKoS0 marked this pull request as ready for review August 30, 2024 18:51
@KiKoS0 KiKoS0 force-pushed the riadh/inn-3330-support-try-catch branch 2 times, most recently from bae2982 to 8330bc0 Compare August 30, 2024 19:25
Base automatically changed from riadh/inn-3325-add-retries-configuration to main September 1, 2024 00:02
An error occurred while trying to automatically change base from riadh/inn-3325-add-retries-configuration to main September 1, 2024 00:02
If an exception is thrown in the `invoke` or `run` function, it will now
be caught and returned as a `StepError` exception. This will allow the
error to be propagated to the caller and handled appropriately.

Transforming the errors into a `StepError` exception is following the
Inngest SDK spec:
https://github.com/inngest/inngest/blob/main/docs/SDK_SPEC.md#522-memoizing-a-step
Inngest JS SDK behavior:
https://github.com/inngest/inngest-js/blob/4f91d9c302592ecc2228914469dd057ae148005b/packages/inngest/src/components/execution/v1.ts#L437-L443
Inngest documentation:
https://www.inngest.com/docs/features/inngest-functions/error-retries/inngest-errors#step-errors
@KiKoS0 KiKoS0 force-pushed the riadh/inn-3330-support-try-catch branch from 8330bc0 to 178e8e2 Compare September 1, 2024 01:16
Copy link
Collaborator

@albertchae albertchae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, let's double check whether avoiding checked exceptions is desired and iterate post merge if necessary

Comment on lines 86 to 87
ResultStatusCode.StepComplete,
ResultStatusCode.StepError,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wonder if it's worth pulling out this set as a constant, maybe calling it something like "terminal" status codes? Fine to leave as is if other SDKs aren't doing that

Comment on lines +130 to +131
is RetryAfterError,
is NonRetriableError,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar comment as the set of ResultStatusCodes, but again not worth doing if other SDKs aren't

Comment on lines 17 to 20
@BeforeAll
static void setup(@Autowired CommHandler handler) {
handler.register("http://localhost:8080");
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can remove this now that #67 is merged right?

@KiKoS0 KiKoS0 merged commit 54b58d7 into main Sep 1, 2024
9 checks passed
@KiKoS0 KiKoS0 deleted the riadh/inn-3330-support-try-catch branch September 1, 2024 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants