From 22804f972f58768dc6af750447a7c69d8513686a Mon Sep 17 00:00:00 2001 From: Claudio Russo Date: Wed, 31 Jul 2024 18:26:20 +0100 Subject: [PATCH] test: testing ability to upgrade after trapping finally (#4641) A trapping finally block will (necessarily) leak a callback table slot. Motoko prevents a canister with non-empty callback table from upgrading, but only unless the canister is stopped. This test verifies that we can recover from a callback table leak (due a faulty finally) by first stopping the canister, upgrading and restarting it. This PR also adds an admonition documenting the danger of trapping finally-blocks and how to recover from them. --- doc/md/reference/language-manual.md | 14 +++- .../ok/try-finally-trap-stop.drun-run.ok | 13 ++++ test/run-drun/ok/try-finally-trap.drun.ok | 14 ++++ test/run-drun/try-finally-trap-stop.mo | 68 +++++++++++++++++++ test/run-drun/try-finally-trap-stop/class.mo | 23 +++++++ test/run-drun/try-finally-trap.drun | 6 ++ .../try-finally-trap/try-finally-trap.mo | 23 +++++++ 7 files changed, 160 insertions(+), 1 deletion(-) create mode 100644 test/run-drun/ok/try-finally-trap-stop.drun-run.ok create mode 100644 test/run-drun/ok/try-finally-trap.drun.ok create mode 100644 test/run-drun/try-finally-trap-stop.mo create mode 100644 test/run-drun/try-finally-trap-stop/class.mo create mode 100644 test/run-drun/try-finally-trap.drun create mode 100644 test/run-drun/try-finally-trap/try-finally-trap.mo diff --git a/doc/md/reference/language-manual.md b/doc/md/reference/language-manual.md index 6467ed8a703..5453af09c30 100644 --- a/doc/md/reference/language-manual.md +++ b/doc/md/reference/language-manual.md @@ -2550,11 +2550,23 @@ Because the [`Error`](../base/Error.md) type is opaque, the pattern match cannot ::: -The `try` expression can be provided with a `finally` cleanup clause to facilitate structured rollback of temporary state changes (e.g. to release a lock). +The `try` expression can be provided with a `finally` cleanup clause to facilitate structured rollback of temporary state changes (e.g. to release a lock). The preceding `catch` clause may be omitted in the presence of a `finally` clause. This form is `try (catch )? finally `, and evaluation proceeds as above with the crucial addition that every control-flow path leaving `` or `` will execute the unit-valued `` before the entire `try` expression produces its result. The cleanup expression will additionally also be executed when the processing after an intervening `await` (directly, or indirectly as `await*`) traps. +:::danger + +The code within a `finally` block should terminate promptly and not trap. +A trapping finally block will fail to free its callback table slot which +can prevent a future upgrade. +In this situation, the canister should be explicitly stopped before re-attempting the upgrade. +In addition, care should be taken to release any resources that may have remained acquired due to the trap. +The canister may be re-started after the upgrade. + +::: + + See [Error type](#error-type). ### Assert diff --git a/test/run-drun/ok/try-finally-trap-stop.drun-run.ok b/test/run-drun/ok/try-finally-trap-stop.drun-run.ok new file mode 100644 index 00000000000..4c4e1f684ba --- /dev/null +++ b/test/run-drun/ok/try-finally-trap-stop.drun-run.ok @@ -0,0 +1,13 @@ +ingress Completed: Reply: 0x4449444c016c01b3c4b1f204680100010a00000000000000000101 +ingress Completed: Reply: 0x4449444c0000 +debug.print: {rts_callback_table_count = 0} +debug.print: trap in finally! +debug.print: trap in finally! +debug.print: {rts_callback_table_count = 1} +debug.print: canister running +debug.print: Canister rrkah-fqaaa-aaaaa-aaaaq-cai trapped explicitly: canister_pre_upgrade attempted with outstanding message callbacks (try stopping the canister before upgrade) +debug.print: canister stopped +debug.print: canister upgraded +debug.print: {rts_callback_table_count = 0} +debug.print: done +ingress Completed: Reply: 0x4449444c0000 diff --git a/test/run-drun/ok/try-finally-trap.drun.ok b/test/run-drun/ok/try-finally-trap.drun.ok new file mode 100644 index 00000000000..721b03b1999 --- /dev/null +++ b/test/run-drun/ok/try-finally-trap.drun.ok @@ -0,0 +1,14 @@ +ingress Completed: Reply: 0x4449444c016c01b3c4b1f204680100010a00000000000000000101 +ingress Completed: Reply: 0x4449444c0000 +ingress Completed: Reply: 0x4449444c0000 +debug.print: trap in finally! +ingress Err: IC0503: Canister rwlgt-iiaaa-aaaaa-aaaaa-cai trapped explicitly: assertion failed at try-finally-trap.mo:8.7-8.19 + +call_on_cleanup also failed: + +Canister rwlgt-iiaaa-aaaaa-aaaaa-cai trapped explicitly: assertion failed at try-finally-trap.mo:12.7-12.19 +debug.print: {rts_callback_table_count = 1} +ingress Completed: Reply: 0x4449444c0000 +ingress Err: IC0503: Canister rwlgt-iiaaa-aaaaa-aaaaa-cai trapped explicitly: canister_pre_upgrade attempted with outstanding message callbacks (try stopping the canister before upgrade) +debug.print: {rts_callback_table_count = 1} +ingress Completed: Reply: 0x4449444c0000 diff --git a/test/run-drun/try-finally-trap-stop.mo b/test/run-drun/try-finally-trap-stop.mo new file mode 100644 index 00000000000..27a104aaa4b --- /dev/null +++ b/test/run-drun/try-finally-trap-stop.mo @@ -0,0 +1,68 @@ +import Lib "try-finally-trap-stop/class"; +import Prim "mo:⛔"; + +// tests that a canister with a leaky callback table +// (due to a trapping finally block) preventing upgrades, +// can still be upgraded after being stopped. +actor { + + let ic00 = actor "aaaaa-aa" : actor { + stop_canister : { canister_id : Principal } -> async (); + start_canister : { canister_id : Principal } -> async (); + }; + + public func go() : async () { + let pre = Prim.cyclesBalance(); + // instantiate Burner actor + Prim.cyclesAdd(pre/2); + var a = await Lib.C(); + let p = Prim.principalOfActor(a); + a := await (system Lib.C) (#upgrade a) (); + await a.show(); + try { + await a.leak(); // leaks a callback in a's callback table + } catch _ { + }; + await a.show(); + Prim.debugPrint("canister running"); + try { + a := await (system Lib.C) (#upgrade a) (); + Prim.debugPrint("canister upgraded"); + assert false; + } catch e { + Prim.debugPrint(Prim.errorMessage (e)); + }; + try { + await ic00.stop_canister({canister_id = p}); + } + catch e { + Prim.debugPrint(Prim.errorMessage (e)); + assert false; + }; + Prim.debugPrint("canister stopped"); + try { + a := await (system Lib.C) (#upgrade a) (); + Prim.debugPrint("canister upgraded"); + } catch e { + Prim.debugPrint(Prim.errorMessage (e)); + assert false; + }; + + try { + await ic00.start_canister({canister_id = p}); + } + catch e { + Prim.debugPrint(Prim.errorMessage (e)); + assert false; + }; + await a.show(); + Prim.debugPrint("done"); + + } + +} + +//SKIP ic-ref-run +//SKIP run-low +//SKIP run-ir +//CALL ingress go "DIDL\x00\x00" diff --git a/test/run-drun/try-finally-trap-stop/class.mo b/test/run-drun/try-finally-trap-stop/class.mo new file mode 100644 index 00000000000..77b832c9f54 --- /dev/null +++ b/test/run-drun/try-finally-trap-stop/class.mo @@ -0,0 +1,23 @@ +import { debugPrint; rts_callback_table_count } = "mo:⛔"; + +actor class C() { + + // leak a callback_table_slot by trapping in finally + public func leak() : async () { + try { + await async (); + } + finally { + debugPrint("trap in finally!"); + assert false + }; + }; + + public func show() : async () { + debugPrint(debug_show + { rts_callback_table_count = rts_callback_table_count() + }); + }; + +}; + diff --git a/test/run-drun/try-finally-trap.drun b/test/run-drun/try-finally-trap.drun new file mode 100644 index 00000000000..74ab7a2ae62 --- /dev/null +++ b/test/run-drun/try-finally-trap.drun @@ -0,0 +1,6 @@ +install $ID try-finally-trap/try-finally-trap.mo "" +upgrade $ID try-finally-trap/try-finally-trap.mo "" +ingress $ID go "DIDL\x00\x00" +ingress $ID show "DIDL\x00\x00" +upgrade $ID try-finally-trap/try-finally-trap.mo "" +ingress $ID show "DIDL\x00\x00" diff --git a/test/run-drun/try-finally-trap/try-finally-trap.mo b/test/run-drun/try-finally-trap/try-finally-trap.mo new file mode 100644 index 00000000000..614245f3d0a --- /dev/null +++ b/test/run-drun/try-finally-trap/try-finally-trap.mo @@ -0,0 +1,23 @@ +import { debugPrint; rts_callback_table_count } = "mo:⛔"; + +actor A { + + public func go() : async () { + try { + await async (); + assert false + } + finally { + debugPrint("trap in finally!"); + assert false + }; + }; + + public func show() : async () { + debugPrint(debug_show + { rts_callback_table_count = rts_callback_table_count() }); + }; + + +}; +