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

Do not report exceptions on long running Excel reads #11916

Merged
merged 7 commits into from
Dec 20, 2024

Conversation

hubertp
Copy link
Collaborator

@hubertp hubertp commented Dec 19, 2024

Pull Request Description

This change introduces two modifications:

  • ClosedByInterruptException is wrapped in InterruptedException instead of RuntimeException
  • when instrumentation encounters InterruptedException it bails early

Having ClosedByInterruptException wrapped in RuntimeException meant that it is being reported as a regular HostException in the engine and to the user. Instead it should be treated specially since we know that it is caused by cancelling a long-running job. Since it is a statically checked exception it has to be declared and the information has to be propagated through various lambda constructs (thanks Java!).

The above change alone meant that an error is not reported for Data.read nodes but any values dependent on it would still report No_Such_Method error when the exception is reported as a value. Hence the early bail out mechanism.
Additional bonus: LS will send PendingInterrupted to GUI that indicates a slightly longer computation that has been interrupted and will be retried.

Closes #11896

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.
  • If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,
    or the Snowflake database integration, a run of the Extra Tests has been scheduled.
    • If applicable, it is suggested to paste a link to a successful run of the Extra Tests.

This change introduces two modifications:
- `ClosedByInterruptException` is wrapped in `InterruptedException`
  instead of `RuntimeException`
- when instrumentation encounters `InterruptedException` it bails early

Having `ClosedByInterruptException` wrapped in `RuntimeException` meant
that it is being reported as a regular `HostException` in the engine and
to the user. Instead it should be treated specially since we know that
it is caused by cancelling a long-running job. Since it is a statically
checked exception it has to be declared and the information has to be
propagated through various lambda constructs (thanks Java!).

The above change alone meant that an error is not reported for
`Data.read` nodes but any values dependent on it would still report
`No_Such_Method` error when the exception is reported as a value. Hence
the early bail out mechanism.
@hubertp hubertp force-pushed the wip/hubert/11896-interrupt-long-running-exec branch from 5c8e588 to d8736a1 Compare December 19, 2024 10:51
@hubertp hubertp added the CI: No changelog needed Do not require a changelog entry for this PR. label Dec 19, 2024
The information could be used in GUI to indicate pending execution that
will take tad longer.
@hubertp
Copy link
Collaborator Author

hubertp commented Dec 19, 2024

Pending some tests but otherwise good to review.

Reduce `PendingInterrupted` to a flag in `Pending`
Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

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

I'm wondering if creating some new unchecked variant of InterruptedException wouldn't make things easier?

On the other hand it feels nice that we have a clear indication which methods are more prone to interrupts (although it feels like any method can be interrupted anyway, no?).

Anyway, looks OK.

@hubertp
Copy link
Collaborator Author

hubertp commented Dec 19, 2024

I'm wondering if creating some new unchecked variant of InterruptedException wouldn't make things easier?

On the other hand it feels nice that we have a clear indication which methods are more prone to interrupts (although it feels like any method can be interrupted anyway, no?).

It's actually fairly mechanical with a decent IDE. And having checked exceptions is actually pretty beneficial in finding bugs.

* @param <E> the type of the checked exception
*/
@FunctionalInterface
public interface FunctionWithException<T, R, E extends Throwable> {
Copy link
Contributor

Choose a reason for hiding this comment

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

We are one step away from inventing monads

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

😆

@hubertp hubertp merged commit d87484b into develop Dec 20, 2024
47 checks passed
@hubertp hubertp deleted the wip/hubert/11896-interrupt-long-running-exec branch December 20, 2024 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

InterruptException/ClosedByInterruptException are still showing up in long running computations
3 participants