Add reentrancy support to Witty #960

jvff · 2023-08-23T02:58:14Z

Motivation

One of the main goals of the Witty crate is to make it easy to export host functions to guest Wasm modules in a reentrant manner, i.e., allowing the host functions to call back to the guest before returning.

Proposal

Allow reentrant functions to be defined in impl blocks that use the #[wit_export] attribute procedural macro by having the functions have a generic type parameter representing the Wasm runtime-specific caller instance. The macro automatically detects this and generates code that will handle the parameter in a special manner.

Since the caller type has to be borrowed by the handler closure, the runtime instance traits must also be implemented for &mut T, not just T anymore.

Test Plan

Some new integration tests are included which have reentrant functions. These functions are called by the guest and act as proxies, calling back to the guest to complete the test.

The tests also cover fallible functions, covering the bug fixed by #959.

Links

Part of #906

Release Plan

All good!
Need to bump the major/minor version number in the next release of the crates.
Need to update the developer manual.
This PR is adding or removing Cargo features.
Release is blocked and/or tracked by other issues (see links above)

Reviewer Checklist

The title and the summary of the PR are short and descriptive.
The proposed solution achieves the goals stated in the PR.
The test plan is reproducible and provides sufficient coverage.
The release plan is adequate.
The commits correspond to distinct logical changes.
The code follows the coding guidelines.
The proposed changes look correct.
The CI is passing.
All of the above!

linera-witty/src/runtime/borrowed_instance.rs

linera-witty/tests/reentrancy.rs

ma2bd · 2023-08-23T06:53:12Z

linera-witty/tests/reentrancy.rs

+    let instance = factory.load_test_module("reentrancy", "operations", |instance| {
+        ExportedOperations::export_to(instance)
+            .expect("Failed to export reentrant operations WIT interface")


(I'm just guessing here but) Would it make sense to adjust the type of the lambda to be able to write this?

factory.load_test_module("reentrancy", "operations", ExportedOperations::export_to)

or perhaps this?

factory.load_test_module::<ExportedOperations>("reentrancy", "operations")

I like the idea, thanks! The only thing is how to handle tests without any exports. I'll create a WithoutExports dummy type and use it as the default type to see how it looks 👍

linera-witty/tests/reentrancy.rs

ma2bd · 2023-08-23T06:59:37Z

linera-witty/test-modules/src/reentrancy/getters.rs

+    witty_macros::test_modules::getters::*,
+};
+
+struct Implementation;


Would it possible to use a global variable to demonstrate modifying a state in successive calls?

I think so. I'll look into it 👍

Done, at least I think? Let me know if the new test is what you were thinking.

ma2bd · 2023-08-23T07:24:59Z

linera-witty/tests/reentrancy.rs

+#[test_case(MockInstanceFactory::default(); "with a mock instance")]
+#[cfg_attr(feature = "wasmer", test_case(WasmerInstanceFactory; "with Wasmer"))]
+#[cfg_attr(feature = "wasmtime", test_case(WasmtimeInstanceFactory; "with Wasmtime"))]
+fn getters<InstanceFactory>(mut factory: InstanceFactory)


Can we start tests with test_ ?

I can do that, but I'm a bit hesitant to do so because the output will start looking like:

test test_getters::with_a_mock_instance ... ok ...

instead of just

test getters::with_a_mock_instance ... ok ...

What do you think?

Well, the usual way to name functions is "do_something", and for tests, ideally "test_that_bla_does_this".
To me, "something" feels like a getter.

For methods and normal functions I agree. But for test functions not so much, because the test functions aren't called by anybody explicitly.

Applied the suggestion because it doesn't matter much anyway 😅

ma2bd · 2023-08-23T07:29:50Z

linera-witty/tests/reentrancy.rs

+pub struct ExportedSetters;
+
+#[wit_export(package = "witty-macros:test-modules", interface = "setters")]
+impl ExportedSetters {


Would it make sense to support this more concise variant?

pub struct ExportedSetters<Caller>(std::marker::PhantomData<Caller>)); impl Default for ExportedSetters<Caller> { ... } #[wit_export(package = "witty-macros:test-modules", interface = "setters")] impl<Caller> ExportedSetters<Caller> where Caller: InstanceForImportedSetters, <Caller::Runtime as Runtime>::Memory: RuntimeMemory<Caller>, { fn set_bool(caller: Caller, value: bool) -> Result<(), RuntimeError> { ... } // .. }

That would be better, thanks! I'll look into it 👍

In a next PR right? (do we need a GH issue?)

Well, I started doing it, but it's a bit more complex than I thought 😅 . I opened #963 to finish it later.

Prepare to allow using the instance types as callers inside reentrant functions.

Check if it has a first parameter with a generic type for receiving the caller instance.

Use only the WIT types in the parameters.

Make reentrant calls possible.

Ensure that the exported host functions can call the guest Wasm instance.

Import and export the same functions to allow the host to act as a proxy in order to test reentrancy.

Use the reentrancy test modules to test reentrancy with real Wasm runtimes.

Add a verb to most test names.

Replace the closure with a type parameter for the `ExportTo` implementation.

Improve debuggability of tests.

Allow some private state to be stored inside the handlers.

Check that a reentrant call back to the guest uses the same internal state from when the guest called the host.

ma2bd

Nice work!

jvff added the enhancement New feature or request label Aug 23, 2023

jvff added this to the Devnet milestone Aug 23, 2023

jvff requested a review from ma2bd August 23, 2023 02:58

jvff self-assigned this Aug 23, 2023

jvff mentioned this pull request Aug 23, 2023

Add a new crate to generate host WIT code #906

Closed

14 tasks

ma2bd reviewed Aug 23, 2023

View reviewed changes

jvff force-pushed the witty-reentrancy branch from 594fd6b to bd73dd7 Compare August 23, 2023 14:06

ma2bd approved these changes Aug 23, 2023

View reviewed changes

jvff added 12 commits August 24, 2023 00:31

Allow using instance traits without moving types

0700a88

Prepare to allow using the instance types as callers inside reentrant functions.

Detect if an exported host function is reentrant

6fa5515

Check if it has a first parameter with a generic type for receiving the caller instance.

Skip reentrant caller parameter in signature

d778f9f

Use only the WIT types in the parameters.

Pass caller argument to reentrant functions

4289c7e

Make reentrant calls possible.

Test reentrant functions using a mock instance

e299a37

Ensure that the exported host functions can call the guest Wasm instance.

Add some test modules to test reentrancy

4191c41

Import and export the same functions to allow the host to act as a proxy in order to test reentrancy.

Test reentrancy using Wasmer and Wasmtime

e4d2aa5

Use the reentrancy test modules to test reentrancy with real Wasm runtimes.

Prefix test names with test_

56db9b0

Add a verb to most test names.

Refactor load_test_module to avoid repeated code

4780c01

Replace the closure with a type parameter for the `ExportTo` implementation.

Show the failed function name in panic message

a0b2adc

Improve debuggability of tests.

Replace function pointer with closure

f1fcc17

Allow some private state to be stored inside the handlers.

Test accesing a guest's global state reentrantly

15358c3

Check that a reentrant call back to the guest uses the same internal state from when the guest called the host.

jvff force-pushed the witty-reentrancy branch from bd73dd7 to 15358c3 Compare August 24, 2023 00:31

jvff mentioned this pull request Aug 24, 2023

Allow generic caller type to be declared once #963

Closed

jvff requested a review from ma2bd August 24, 2023 00:49

ma2bd approved these changes Aug 24, 2023

View reviewed changes

jvff merged commit 2ab872b into linera-io:main Aug 24, 2023

jvff deleted the witty-reentrancy branch August 24, 2023 01:20

jvff mentioned this pull request Aug 25, 2023

Simplify generics for exporting reentrant functions with Witty #965

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add reentrancy support to Witty #960

Add reentrancy support to Witty #960

jvff commented Aug 23, 2023 •

edited by ma2bd

Loading

ma2bd Aug 23, 2023

jvff Aug 23, 2023

jvff Aug 24, 2023

ma2bd Aug 23, 2023

jvff Aug 23, 2023

jvff Aug 24, 2023

ma2bd Aug 23, 2023

jvff Aug 23, 2023

ma2bd Aug 23, 2023

jvff Aug 24, 2023

ma2bd Aug 23, 2023

jvff Aug 23, 2023

ma2bd Aug 23, 2023

jvff Aug 24, 2023

ma2bd left a comment

Add reentrancy support to Witty #960

Add reentrancy support to Witty #960

Conversation

jvff commented Aug 23, 2023 • edited by ma2bd Loading

Motivation

Proposal

Test Plan

Links

Release Plan

Reviewer Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ma2bd left a comment

Choose a reason for hiding this comment

jvff commented Aug 23, 2023 •

edited by ma2bd

Loading