Resultify AVM

[cdmistman]

fixes #472

[aguillenv]

These comments are for future debugging?

[cdmistman]

yes - I didn't remove it since it's been there for a while (and could prove useful in the future)

[aguillenv]

Hmmm will think how to avoid this and be transparent for us

[cdmistman]

sounds good. I brought it back in the next commit, but I keep forgetting the 1 extra step after git add .

[dfellis]

Undo these changes

[dfellis]

What's going on here?

[cdmistman]

with this, we can use BuiltInEvents::START as i64

[dfellis]

This whole branch is known (to us) to never happen because of the way the opcode fragments work. It would be nice to be able to assert this with the type during the fragment generation, perhaps having another enum for the "fragment type" wrapping around the vec and only allowing the particular opcode function enum we care about within each of these outer enums and being able to avoid this looping check here?

Not something for this PR, but it does bother me that we have to define an error path here that will never be triggered and that we have to pay a price doing these useless checks.

[cdmistman]

I think there's a way to unify the function types of the different opcode kinds, so then we can entirely get rid of these checks and only use the additional information for building up the instruction fragments. I imagine it'd be a struct with 2 fields, and running them would be much faster.

The new opcode defs would look like:

pub enum OpcodeKind {
  CPU,
  IO,
  UnpredCPU,
}
pub struct Opcode {
  name: &'static str,
  kind: OpcodeKind,
  exec: fn(&[i64], Arc<HandlerMemory>) -> Box<Pin<VMResult<Arc<HandlerMemory>>>>,
}

so execution can be done with purely an iterator (simplified):

handler.instructions
  .iter()
  .map(|fragment| {
    fragment
      .iter()
      .map(|(op, instrs)| exec(&instrs, hmem.fork()))
      .collect::<UnorderedFutures<VMResult<Vec<Arc<HandlerMemory>>>>>>()
  })
  .collect::<OrderedFutures<VMResult<Vec<Arc<HandlerMemory>>>>>()

[dfellis]

So the CPU-bound opcodes can return an event payload, they don't return the HandlerMemory.

However, I think that weirdness is used only by the emitop opcode, so if we can refactor things to let event emission be done specially by that opcode and not require checking the return value, that'd be great. But what I'd prefer is that CPU opcodes still have a special return type, it's just that it's now void and we don't have to do any conditional checking on every single CPU opcode call, which is probably a significant perf impact.

[dfellis]

Why this change?

[cdmistman]

the service function could return a VMError since it has to work with the memory of the event that handles each request, so I allow the hyper service to propagate such errors - although we could handle the error directly in the service function?

[dfellis]

So for the web server I would prefer it is infallible and if it encounters any VMError responses it returns a 500 Internal Server Error to the end user with some sort of short message indicating an unexpected failure within the AVM itself? Simply crashing out will make it harder to track down what's going wrong, I think.

[dfellis]

We're wrapping another layer around HandlerMemory here.

You know when we want to improve performance we're going to be undoing a lot of this and going back to "we have thoroughly tested/audited our code so we don't have to pay a runtime penalty for this safety" :/

[cdmistman]

that makes sense, although many of these checks are happening anyways when we call unwrap or expect. This is just to allow us to prevent these panics and recover from them if the daemon is running.

[dfellis]

Why switch from the for in loop to this if it depends on discourage usage of a built-in type?

[cdmistman]

the for in loop from before had a non-constant number of allocations - this iterator implementation has 1 allocation total, guaranteed (on iter() - skip doesn't allocate, IntoIter::new recasts the array from chars.to_ne_bytes to an iterator, and collect inherits the allocation from iter())

[cdmistman]

(side note: impl IntoIterator for [T; N] will be stabilized soon, but it'll actually call impl IntoIter for Slice, which will result in dropping the array while returning the iterator on a slice of the array, which is a compiler error. In edition 2021, this will delegate to array::IntoIter, which won't be deprecated)

[dfellis]

These opcodes sound very confused and unsure of themselves "read fixed argument 0? I guess? Then write it as a bool to argument 2? Ok, nothing else."

[dfellis]

This is good, but makes me wonder what we can/should do at compile time for this. A bad regex should be detectable during the compilation phase.

[cdmistman]

unfortunately not :/ regex does some heap allocations that can't be done at compile time* - see https://docs.rs/regex/1.4.5/src/regex/exec.rs.html

* allocations in const-fns is planned, per rust-lang/const-eval#20

[dfellis]

Oh, I was thinking something like "Should we have a fake rust file that we inject the users' regex into and test if it parses correctly and just emit an error during compilation when the regex string is static"

But maybe the correct answer here is: "the regex API in Alan needs to be changed to be a Result and we return an Error to the user if it fails to build correctly"

[cdmistman]

ah I see - we could also ship ripgrep with our compiler binary, and when we detect regex usage we

1. create an empty file
2. run rg on that empty file using the user's regex
3. continue compiling if the exit code is 0 or throw a compiler error if the exit code is non-zero

We could also shave regex off our dependency tree and save some space in the binary size by just shipping ripgrep in the avm regardless of if the compiler is there, and every time the user wants to do a regex query we do the same but on a file containing the input.

[cdmistman]

In all seriousness though, we might be able to pre-compile the Regex in the compiler by calling regex as a wasm module, store the object as a Buffer, and include it in the aga/agc output, but that would take a solid amount of time to get working...

[cdmistman]

actually it's not worth it to store the memory representation of regex::Regex (to get past the platform differences you'd have to include a wasm runtime - which could be absolutely minimal - to compile the wasm memory to native memory, but the issue lies in Rust's lacking of a stable ABI), but we could still run it from wasm to validate the input

[dfellis]

I think all of this makes it clear that we need to change the regex-related APIs in Alan to be Result-based so it can provide the failure at runtime. At least the user will need to have defined some code to handle what happens if it can't parse the regex, though that is almost certainly going to be a logic bug. :/

[aguillenv]

Why exit code 2 and not 1?

[cdmistman]

that was what we were using in the majority of cases where we did eprintln!() and then exit()

[aguillenv]

Hmmm interesting, does this work as unwrap_or?

[cdmistman]

no, since ok_or still returns the Result, whereas unwrap_or is the same as getOr in Alan (if it's an Err, returns an Ok with the given default value)

[aguillenv]

Got it

[aguillenv]

the chances to go through this brach are quite small right? If would represent an important error and difficult to track might be interesting to log it

[cdmistman]

yes, but I'll let the daemon error-catching PR handle that

[depombo]

LGTM once all tests are passing (only the AVM style right now?) and we verify there is no change in behavior/functionality for the AVM

[dfellis]

@cdmistman it appears that there was another accidental behavior change here, the double-send guard on http server responses apparently broke: https://github.com/alantech/alan/pull/496/checks?check_run_id=2403147748#step:4:1373

[cdmistman]

there was another accidental behavior change here, the double-send guard on http server responses apparently broke

As mentioned offline, this is from observing the results from the JS runtime, indicating that there isn't a change in behavior, but rather a race condition in our http tests.