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.

Sign up for GitHub

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

Jump to bottom

CI is red #173

Closed
peterhuene opened this issue Nov 2, 2022 · 2 comments · Fixed by #175
Closed

CI is red #173

peterhuene opened this issue Nov 2, 2022 · 2 comments · Fixed by #175

Comments

@peterhuene
Copy link
Member

There were breaking changes upstream that impacted the C API, which are breaking the .NET CI tests.
@kpreisser
Copy link
Contributor

kpreisser commented Nov 3, 2022 •
edited
Loading

Hi, I'm not sure if I fully understand these changes (I don't know Rust), but I gather that host traps that are created with wasmtime_trap_new (and e.g. a WASI exit status set by WASI's proc_exit) are now returned as error instead of as trap, so e.g. wasmtime_func_call returns an non-null wasmtime_error_t*, which causes a WasmtimeException to be thrown instead of a TrapException.
Only traps that are caused in wasm code (that map to a wasmtime_trap_code_enum value) are returned as actual traps (wasm_trap_t*).

However, I noticed that for test Wasmtime.Tests.FuelConsumptionTests.ItThrowsOnCallingImportMethodIfNoFuelAdded, it seems that a trap (wasm_trap_t*) is returned, but calling wasmtime_trap_code fails:

 Wasmtime.Tests.FuelConsumptionTests.ItThrowsOnCallingImportMethodIfNoFuelAdded
   Source: FuelConsumptionTests.cs line 126
   Duration: 160 ms

  Message: 
Expected a <Wasmtime.TrapException> to be thrown, but found <System.Runtime.InteropServices.SEHException>: "
"System.Runtime.InteropServices.SEHException with message "External component has thrown an exception."
     at Wasmtime.TrapException.Native.wasmtime_trap_code(IntPtr trap, TrapCode& exitCode)
     at Wasmtime.TrapAccessor.get_TrapCode() in C:\Users\developer4\Desktop\WebAssembly-Tests\wasmtime-dotnet\src\TrapException.cs:line 128
     at Wasmtime.TrapException.FromOwnedTrap(IntPtr trap, Boolean delete) in C:\Users\developer4\Desktop\WebAssembly-Tests\wasmtime-dotnet\src\TrapException.cs:line 306
     at Wasmtime.Function.Invoke(ReadOnlySpan`1 arguments) in C:\Users\developer4\Desktop\WebAssembly-Tests\wasmtime-dotnet\src\Function.cs:line 2141
     at Wasmtime.Function.Invoke() in C:\Users\developer4\Desktop\WebAssembly-Tests\wasmtime-dotnet\src\Function.cs:line 2082
     at Wasmtime.Tests.FuelConsumptionTests.<>c__DisplayClass19_0.<ItThrowsOnCallingImportMethodIfNoFuelAdded>b__0() in C:\Users\developer4\Desktop\WebAssembly-Tests\wasmtime-dotnet\tests\FuelConsumptionTests.cs:line 131
     at FluentAssertions.Specialized.ActionAssertions.InvokeSubject()
     at FluentAssertions.Specialized.DelegateAssertions`2.InvokeSubjectWithInterception()
.

Edit: Also, the test host process seems to crash randomly as seen here (Linux) and here (Windows). This seems to occur due to the above SEHException; when I remove the FuelConsumptionTests, the random crashes no longer happen.

Regarding the host errors, I think this means that the functionality from #172 would need to be shifted from TrapException to WasmtimeException, so that the cause can then be stored as inner exception of the WasmtimeException.
(Another possibility would be to directly rethrow the stored exception in such a case, so that for the user it would look like the original .NET exception bubbled through the whole stack. Though that would hide the wasm backtrace.)

However, I'm not sure how the error handling in wasmtime works: E.g. in the case when we return a host trap at the .NET callback, will the returned wasmtime_error_t* at wasmtime_func_call always correspond to that set host trap, or could it also happen that another error is returned that is not related to the created trap? In that case, we might incorrectly store the .NET callback exception as InnerException of the WasmtimeException when the error has a different cause than the trap.

What do you think?

Regarding the WASI exit status, I'm not sure how it can be retrieved now, as for calling wasmtime_trap_exit_status we would need to have a wasm_trap_t*, not an wasmtime_error_t*.

Additionally, I noticed that some tests in the CI seem to have failed due to an out of memory condition:

  Failed Wasmtime.Tests.Memory64AccessTests.ItThrowsForOutOfBoundsAccess [1 ms]
  Error Message:
   Wasmtime.WasmtimeException : Insufficient resources: VirtualAlloc failed: The paging file is too small for this operation to complete. (os error 1455)

Maybe the CI runner doesn't have enough RAM to always allocate a 4 GiB Memory? (Should we disable these Memory access tests for CI)?

Thanks!

kpreisser added a commit to kpreisser/wasmtime-dotnet that referenced this issue Nov 3, 2022
@kpreisser
Update some tests for the new trap behavior.
b7dbedf 
See bytecodealliance#173

Currently, tests that are still failing are the following:
- ExitTrapTests: It is not clear how to get the WASI exit code from a `wasmtime_error_t*` instead of an `wasmtime_trap_t*` via the C API.
- FuelConsumptionTests: `wasmtime_trap_code` unexpectedly fails (SEHException) even though a trap was returned.
- TrapTests: The tests introduced with bytecodealliance#172 fail as the inner exception would need to be set on the WasmtimeException instead of TrapException. Need to check how to solve this.
@kpreisser kpreisser mentioned this issue Nov 3, 2022
Merged
@kpreisser
Copy link
Contributor

kpreisser commented Nov 7, 2022 •
edited
Loading

Regarding the WASI exit status, I'm not sure how it can be retrieved now, as for calling wasmtime_trap_exit_status we would need to have a wasm_trap_t*, not an wasmtime_error_t*.

This should be fixed with bytecodealliance/wasmtime#5215.

peterhuene added a commit that referenced this issue Nov 10, 2022
@kpreisser @peterhuene
Update implementation and tests for the trap redesign (#175)
fe87f2f 
* Update some tests for the new trap behavior.

See #173

Currently, tests that are still failing are the following:
- ExitTrapTests: It is not clear how to get the WASI exit code from a `wasmtime_error_t*` instead of an `wasmtime_trap_t*` via the C API.
- FuelConsumptionTests: `wasmtime_trap_code` unexpectedly fails (SEHException) even though a trap was returned.
- TrapTests: The tests introduced with #172 fail as the inner exception would need to be set on the WasmtimeException instead of TrapException. Need to check how to solve this.

* Update for the changes in bytecodealliance/wasmtime#5215.

This moves the ExitStatus from TrapException/TrapAccessor to WasmtimeException.

* Follow-Up: Add a Frames property to WasmtimeException which allows to retrieve the frames for a wasmtime_error_t* using the new wasmtime_error_wasm_trace function.

TODO: Add tests.

* Use nuint instead of UIntPtr for sizes (size_t) as they don't actually represent a pointer, but a platform-sized integer.

Note that nint/nuint in C# will behave like regular numeric types like int/uint, so e.g. the checked/unchecked context is respected, which is not the case with IntPtr/UIntPtr until .NET 7.

* Move the functionality of passing the original .NET exception as InnerException from the TrapException to the WasmtimeException, since host-created traps (with wasm_trap_new) are now returned as error (wasmtime_error_t*).

While theoretically it could happen that the returned wasmtime_error_t* has actually another cause than the error returned at the host-to-wasm transition, I assume this is not the case, similar as to a .NET exception that would just bubble up the stack once thrown.

* Fix the definition of wasmtime_trap_code to match the signature from trap.h header file.

See GHSA-h84q-m8rr-3v9q

Previously, we were passing a pointer to an Int32 (4-byte buffer), so the definition actually matched the Rust code, and therefore prior versions of wasmtime-dotnet are not impacted by that issue.

* Add TrapCode.OutOfFuel.

See bytecodealliance/wasmtime#5230

* Follow-Up: Update for the changed exception message.

* Update the consume-fuel example.

This commit updates the consume-fuel example to catch `WasmtimeException`
instead of `TrapException`.

The change is due to the `ConsumeFuel` call returning an error when it
fails to consume the given amount of fuel and that propagates properly now.

Co-authored-by: Peter Huene <phuene@fastly.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Update implementation and tests for the trap redesign kpreisser/wasmtime-dotnet
2 participants
@peterhuene @kpreisser