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

feat: finally to also clean up after trapping continuations #4507

Merged
merged 207 commits into from
Jul 25, 2024
Merged

Conversation

ggreif
Copy link
Contributor

@ggreif ggreif commented Apr 22, 2024

Introduces an optional finally clause to try expressions that can be used for disciplined (structured) resource management. The clause comprises a unit-valued expression (or block) that may not perform sends (or similar). It gets executed for all control paths that leave the try-expression, i.e. return, break, etc. code paths. For nested try expressions the cleanups are performed in innermost to outermost order.

For last-resort cleanups (i.e. code paths trapping after an await) the replica invokes the callback registered as ic0.call_on_cleanup, and this will result in the execution of (exclusively) the finally blocks in (dynamic) scope. This is a new mechanism in Motoko, which was not possible to achieve earlier, and puts certain resource deallocations (e.g. of acquired locks) under the programmer's control.


TODO:

  • invoke cleanup from the interpreter
  • make catch optional (non-trapping when missing) — Allow try without catch #4600
    • well, this was not really needed as pattern-match failure would just re-throw, so we were fine. Optimised, though.
  • remaining FIXMEs (arrange.ml, etc.)
  • Triv in try (respect edges)
    • fix fallout
  • forbid control-flow edges out of finally blocks
  • remove all aliasing from IR
    • use meta (would assert currently, thus causing minor code duplication) — could be follow-up PR
  • handle Throw and regular continuations in await.ml (instead of desugaring)
    • adjust (AST, IR)-interpreter
    • ❗️this is necessary when there is no catch, as the exception will escape
  • how are label continuations invoked in the backend? (I need to know for the last-resort callback)
    • have a new context key sort Cleanup, stack up continuations of finally blocks only
    • extend the kr in await.ml to krc (CPSAwait), pass Cleanup continuations as c
    • add new primitive to compiler to set the last-resort field (not needed)
    • in async.ml lower CPSAwait to that too
    • adapt check_ir.ml, etc.
  • tests with await*
  • fix up the syntax part
  • do ... finally (when there is no catch clause necessary)
    • means: TryE needs cases list option
  • type checking for the finally clause
    • unit result
    • trivial effect (actually Claudio made this capability-based)
    • that the try block has await effect — nope!
  • other type than unit for try
  • OtherPrim "call_raw" + tests — nope!
  • cps_asyncE similarly to CallPrim/3 — nope!
  • highlighting of finally
  • Changelog (breaking change, new keyword)
  • Docs
  • adapt best practices: https://internetcomputer.org/docs/current/developer-docs/security/security-best-practices/inter-canister-calls#recommendation
  • adapt Motoko examples where locking happens

@ggreif ggreif added feature New feature or request language design Requires design work interpreter DO-NOT-MERGE labels Apr 22, 2024
Copy link

github-actions bot commented Apr 22, 2024

Comparing from 5fd4e2e to 78a8659:
In terms of gas, 4 tests regressed, 1 tests improved and the mean change is +0.0%.
In terms of size, 5 tests regressed and the mean change is +0.1%.

ggreif and others added 10 commits July 23, 2024 14:04
Changelog.md Outdated Show resolved Hide resolved
@@ -313,6 +315,21 @@ actor A {
try await t6t() catch _ {};
try await t6d() catch _ {};
try await t8t() catch _ {};

// other side problem tests
try await async {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I think this is behaving correctly. Maybe refactor into a separate subtest/function (like the others)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll do this in a separate PR.

Copy link
Contributor

@crusso crusso left a comment

Choose a reason for hiding this comment

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

See comments

src/ir_passes/await.ml Outdated Show resolved Hide resolved
@crusso crusso self-requested a review July 24, 2024 11:24
Copy link
Contributor

@crusso crusso left a comment

Choose a reason for hiding this comment

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

There's something fishy in await.ml. Hold off merging for a bit

anticipating 0.12.0
@ggreif ggreif requested a review from crusso July 24, 2024 15:32
@ggreif ggreif added the automerge-squash When ready, merge (using squash) label Jul 24, 2024
Copy link
Contributor

@crusso crusso left a comment

Choose a reason for hiding this comment

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

We can merge but I think it would be good to do the follow up PR addressing code duplication before releasing.

@mergify mergify bot merged commit 132b4b4 into master Jul 25, 2024
10 checks passed
@mergify mergify bot removed the automerge-squash When ready, merge (using squash) label Jul 25, 2024
@mergify mergify bot deleted the gabor/finally branch July 25, 2024 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request interpreter language design Requires design work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants