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

Problems catching a Panic thrown within a TCO loop #9251

Closed
radeusgd opened this issue Mar 2, 2024 · 9 comments · Fixed by #9705
Closed

Problems catching a Panic thrown within a TCO loop #9251

radeusgd opened this issue Mar 2, 2024 · 9 comments · Fixed by #9705
Assignees
Labels

Comments

@radeusgd
Copy link
Member

radeusgd commented Mar 2, 2024

The following program demonstrates the issue:

from Standard.Base import all
import Standard.Base.Errors.Illegal_State.Illegal_State

# Function with TCO
foo1 x =
    if x > 10 then Panic.throw (Illegal_State.Error "foo1") else
        @Tail_Call foo1 x+1

# Same loop but without TCO
foo2 x =
    if x > 10 then Panic.throw (Illegal_State.Error "foo2") else
        foo2 x+1

main =
    handle caught =
         IO.println "Caught "+caught.payload.to_text

    IO.println "without TCO:"
    Panic.catch Any handler=handle <| foo2 0
    IO.println "now, with TCO:"
    Panic.catch Any handler=handle <| foo1 0
    IO.println "done (will not be printed 🙁)"

Actual behaviour

without TCO:
Caught (Illegal_State.Error 'foo2' Nothing)
now, with TCO:
Execution finished with an error: Illegal State: foo1
        at <enso> Panic.throw(Internal)
        at <enso> catching.foo1<arg-1>(catching.enso:6:20-59)
        at <enso> catching.foo1(catching.enso:6-7)
        at <enso> catching.main(catching.enso:21:5-44)

We can see that the Panic thrown from foo1 is not caught in main and instead fails the whole function.

The only difference between foo1 and foo2 is that foo1 relies on @Tail_Call.

Expected behaviour

Both functions should behave the same and it should be possible to catch the panic in both cases.

without TCO:
Caught (Illegal_State.Error 'foo2' Nothing)
now, with TCO:
Caught (Illegal_State.Error 'foo1' Nothing)
done (will not be printed 🙁)
@radeusgd
Copy link
Member Author

radeusgd commented Mar 2, 2024

We've been observing panics falling through the test suite and crashing the suite from time to time. I suspect that this might also have been an instance of this issue.

radeusgd added a commit that referenced this issue Mar 2, 2024
mergify bot pushed a commit that referenced this issue Mar 4, 2024
- Adds a missing finalizer to a test
- Adds a workaround for #9251 bug
@hubertp
Copy link
Contributor

hubertp commented Mar 5, 2024

Anyone who is looking into it should check if there is no duplicate for it already. I believe this was a known issue.

@Akirathan
Copy link
Member

Anyone who is looking into it should check if there is no duplicate for it already. I believe this was a known issue.

@hubertp I was not able to find any potential duplicates.

@enso-bot
Copy link

enso-bot bot commented Apr 15, 2024

Pavel Marek reports a new STANDUP for today (2024-04-15):

Progress: - Debugging the issue.

  • Where is the TailCallException thrown and caught?
  • Where is the PanicException thrown?
  • While catching the TailCallException, some nodes are replaced and the CatchPanicNode is discarded.
  • It seems that the problem is more complicated - the LoopOptimisation nodes are constructed in a weird places. It should be finished by 2024-04-22.

@enso-bot
Copy link

enso-bot bot commented Apr 29, 2024

Pavel Marek reports a new 🔴 DELAY for today (2024-04-29):

Summary: There is 14 days delay in implementation of the Problems catching a Panic thrown within a TCO loop (#9251) task.
It will cause 0 days delay for the delivery of this weekly plan.

Delay Cause: Getting back to this issue after more than a week

@enso-bot
Copy link

enso-bot bot commented Apr 29, 2024

Pavel Marek reports a new STANDUP for today (2024-04-29):

Progress: - Merged the Private constructor PR - addressed last issues.

  • Write another failing test that does not use the <| method.
  • Digging deep into the issue, printing stack traces, writing descriptions. It should be finished by 2024-05-06.

@enso-bot
Copy link

enso-bot bot commented Apr 30, 2024

Pavel Marek reports a new STANDUP for today (2024-04-30):

Progress: - Helping Dmitry with launching ydoc server as a module.

  • Rehersal of the Geecon JPMS presentation It should be finished by 2024-05-06.

@mergify mergify bot closed this as completed in #9705 May 1, 2024
@enso-bot
Copy link

enso-bot bot commented May 1, 2024

Pavel Marek reports a new STANDUP for today (2024-05-01):

Progress: - More verbose logging for the UnresolvedMethod(==) bug

  • Finished the Panic catching in TCO-optimized loop by simply executing the action parameter of Panic.catch as non-tail. It should be finished by 2024-05-06.

@enso-bot
Copy link

enso-bot bot commented May 2, 2024

Jaroslav Tulach reports a new STANDUP for yesterday (2024-05-01):

Progress: - trying to understand TCOs + fix: https://github.com/enso-org/enso/pull/9705/files#r1586038166

GitHub
Fixes #9251 Pull Request Description In certain cases, when the action of Panic.catch is tail-call-optimized (via @Tail_Call) annotation, the panic is not caught. Fixed by ensuring that the action ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: 🟢 Accepted
Development

Successfully merging a pull request may close this issue.

4 participants