Trap Result #151

martindevans · 2022-09-14T23:28:33Z

Added in 4 new Result types (Result, ResultWithBacktrace, Result<T>, ResultWithBacktrace<T>) which provide a better method for trap handling than exceptions. This entire new system is opt-in, the function wrappers accept these types instead of the actual return type and then, if a trap happens, it is returned in this result struct instead of as an exception.

Result represents a function with no return value, which does not capture the trap backtrace.
ResultWithBacktrace represents a function with no return value, which captures the trap backtrace.
Result<T> represents a function which returns T, which does not capture the trap backtrace.
ResultWithBacktrace<T> represents a function which returns T, which captures the trap backtrace.

Also some associated to the TrapException:

Added a TrapCode Type property, which was an obvious omission from traps
Modified FunctionOffset and ModuleOffset of the TrapFrame to use nuint instead of int (which could cause an arithmetic overflow) (see Overflow Exception In Trap Handling #150)

…lt<T>`, `ResultWithBacktrace<T>`) which provide a better method for trap handling than exceptions. function wrappers accept these types instead of the actual return type and then, if a trap happens, it is returned in this result struct instead of as an exception. - `Result` represents a function with no return value, which does not capture the trap backtrace. - `ResultWithBacktrace` represents a function with no return value, which captures the trap backtrace. - `Result<T>` represents a function which returns `T`, which does not capture the trap backtrace. - `ResultWithBacktrace<T>` represents a function which returns `T`, which captures the trap backtrace. Also some associated to the `TrapException`: - Added a `TrapCode Type` property, which was an obvious omission from traps - Modified `FunctionOffset` and `ModuleOffset` of the `TrapFrame` to use `nuint` instead of `int` (which could cause an arithmetic overflow)

martindevans · 2022-09-14T23:36:15Z

I'm not sure what the macos failure is caused by, it looks like something wrong with the CI process itself?

willhausman · 2022-09-14T23:50:13Z

That error looks an awful lot like the crashes I would see when testing throwing exceptions in WasmerSharp and Wasmtime to compare. It happened locally on mac but not windows. Frustrating to discover. I only mention it to caution against assuming the CI itself is at fault.

martindevans · 2022-09-15T00:12:19Z

Unfortunately I don't have access to a mac to test on. Any hints on what fixed the problem for you that I could try?

willhausman · 2022-09-15T00:58:39Z

I can try pulling your fork and see what happens for me later tonight or tomorrow. My solution was to stop using WasmerSharp. Useful, right? Ha. It just feels too much of a coincidence, given you are working with the traps, and I was testing how the two runtimes handled exceptions.

willhausman · 2022-09-15T01:18:27Z

I can confirm dotnet test Wasmtime.sln does exhibit this same behavior for me locally on both x64 and ARM Macs. Sorry I can't provide any more useful advice beyond my test results at the moment. I understand the traps at a conceptual level, but have not sifted through the implementation thoroughly enough to understand exactly what is happening.

martindevans · 2022-09-15T01:31:27Z

Thanks for the testing, that at least confirms it isn't just a flakey CI problem.

martindevans · 2022-09-15T01:39:30Z

If possible could you share the log from the test on your Mac? Hopefully that will contain more useful information than the CI log.

martindevans · 2022-09-15T13:47:24Z

I have worked around the CI failure on MacOS by replacing the stackoverflow errors with integer div zero errors. I think the problem with MacOS+Stackoverflow is a bug in wasmtime-dotnet, independent of the changes in this PR.

I've pushed up #152 which includes a test that I expect to fail on MacOS to help identify the issue.

peterhuene · 2022-09-19T16:32:18Z

Hi @martindevans! I'll review this today and dig into the stack overflow trap crashing the test host.

peterhuene

This looks great and I love the opt-in nature of it!

Just a few comments; almost all of them are minor nits.

src/Function.cs

src/Result.cs

src/Function.cs

src/Result.cs

peterhuene · 2022-09-19T22:24:46Z

tests/TrapTests.cs

+            var result = run();
+
+            result.Type.Should().Be(ResultType.Trap);
+            result.Trap.Should().Be(TrapCode.IntegerDivisionByZero);


Should we be asserting here (and in ItReturnsATrapCodeGenericResult) that the frames are empty?

Unless I'm misunderstanding what you're asking for I don't think that's possible in those two cases. They're both Result types (not ResultWithBacktrace) so the only thing available is the TrapCode, the backtrace is not available.

Oh right, the name Trap threw me off here. It should probably be, at the minimum, TrapCode so it doesn't seem like the whole trap exception is being stored.

Stepping back, though, should we not store a TrapException for the non-backtrace capturing Result types too, just without Frames populated?

I think it'd be useful to get the message in addition to the code, just without the backtrace.

Good point on the naming, I'll change that now 👍

Regarding the TrapException/message my intention was to have the Result be as lightweight as possible, avoiding allocating a TrapException and the message string. Personally I would prefer to keep it that way.

Maybe the problem here is naming - Result (the simpler name) is the opt-in to a lighter weight version with no useful debugging information. Perhaps instead they should be named Result (with a backtrace) and ResultWithoutAnyUsefulInformation (pending a better name). That way it's probably more obvious that the version with the backtrace is the one you "should" use?

Having said all that, I'm wondering if another possible way to do this is instead of having some special "blessed types" which can interact with traps we should instead detect if the return type implements a special interface and then do things through that. That way we can ship a single Result type with backtraces/messages etc, but there's a door to hook into the trap system (in a safe way of course) and get just what you need. If you don't mind holding off on this PR for a while I'll experiment with that approach to see if it's feasible.

Sounds good, I'll hold off on the PR to see what you come up with.

I'm also good with landing this PR if that's what you decide is best.

martindevans · 2022-09-19T22:47:23Z

I've pushed up a commit that should address all feedback, except that last point. Thanks for the excellent review, as usual 👍

martindevans · 2022-09-21T15:58:10Z

I've pushed up #153 which further develops this idea with a system for custom result types. Although the details of how it works are more complex I think it offers a better user experience for the 99% of users who can just use ActionResult/FunctionResult<T> and don't need to think about if they want a backtrace etc. It also offers a bettter experience for the remaining 1% of users with weird requirements, since they can implement fully custom result types instead of having to pick between whatever options we happen to offer.

peterhuene · 2022-09-26T20:15:10Z

Closing this as superseded with #153.

martindevans added 2 commits September 15, 2022 00:26

When committing code, remember to actually include new files.

f6e4031

Replaced stackoverflows in trap tests with div zero

db4c237

martindevans mentioned this pull request Sep 15, 2022

Trap Stackoverflow Test #152

Open

peterhuene reviewed Sep 19, 2022

View reviewed changes

Addresses PR feedback

e6f4a48

martindevans added 2 commits September 20, 2022 00:18

Renamed Trap to TrapCode (review feedback)

528d580

Oops, changing tests to fit renaming of that property

07958c5

peterhuene approved these changes Sep 20, 2022

View reviewed changes

peterhuene closed this Sep 26, 2022

martindevans deleted the trap_result branch November 30, 2022 14:30

Trap Result #151

Trap Result #151

Uh oh!

Conversation

martindevans commented Sep 14, 2022

Uh oh!

martindevans commented Sep 14, 2022

Uh oh!

willhausman commented Sep 14, 2022

Uh oh!

martindevans commented Sep 15, 2022

Uh oh!

willhausman commented Sep 15, 2022

Uh oh!

willhausman commented Sep 15, 2022

Uh oh!

martindevans commented Sep 15, 2022

Uh oh!

martindevans commented Sep 15, 2022

Uh oh!

martindevans commented Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peterhuene commented Sep 19, 2022

Uh oh!

peterhuene left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

peterhuene Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

martindevans Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

peterhuene Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

martindevans Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

peterhuene Sep 20, 2022

Choose a reason for hiding this comment

Uh oh!

martindevans commented Sep 19, 2022

Uh oh!

martindevans commented Sep 21, 2022

Uh oh!

peterhuene commented Sep 26, 2022

Uh oh!

Uh oh!

martindevans commented Sep 15, 2022 •

edited

Loading