wasmCloud / wascc-host

Library for hosting actors and capability providers in a host process
Apache License 2.0
202 stars 15 forks source link

Invocation error not being propagated #102

Closed jose-acevedoflores closed 3 years ago

jose-acevedoflores commented 3 years ago

If the test is a bit intrusive or redundant I can remove it and the dummy-actor files . I linked the fs provider so I could trigger the removal of a non existent container. Also added that resources/dummy-actor in the tests folder. I'm not sure what is the convention in rust for supplemental test files.

autodidaddict commented 3 years ago

Can you do a cargo fmt on this PR? The format check failed the build

jose-acevedoflores commented 3 years ago

Can you do a cargo fmt on this PR? The format check failed the build

I'm on it.

jose-acevedoflores commented 3 years ago

I noticed the first checks failed on lattice mode. I'll run it locally to see if I can figure out why it blows up. I've never run lattice mode so it might take awhile.

jose-acevedoflores commented 3 years ago

That was a fun experiment.

So I believe since my integration test was using the default fs-provider, in lattice mode, it was binding to the wrong instance(still not sure why or if this was really the case). I added a named binding plus a dummy provider with just one method implemented (remove_container). Before it was relying on the fs provider's remove_container method returning an error via the remove_dir failing. Now the remove_container in the dummy provider returns an error directly and that fixed it.

I was thinking about squashing the commits but not sure what's the policy with regards to rebasing for the project so if it's needed let me know.

Side note: setting up lattice mode was a breeze, pretty awesome stuff!

jose-acevedoflores commented 3 years ago

Well, I'm no longer confident my integration test is ready. I can't replicate it in my machine when I run: cargo test --features "lattice bin manifest wasmtime" --test integration fs_host_error -- --test-threads=1

In my setup I have one host running that binds the real fs-provider in lattice mode. When I run the test using the default binding I do see it reaching the real fs-provider but when I added the binding name it stopped. I thought that would fix it but this last run failed and the code was effectively the same as commit c428aa5 (that passed) with some messaging changes for clarity.

If there doesn't seem to be an obvious solution I can back out the test, I'm still getting familiarized with the code base so this test might not be good.

jose-acevedoflores commented 3 years ago

I was finally able to replicate on my machine. I lowered my LATTICE_RPC_TIMEOUT_MILLIS down to 10 millis and it triggers the <I/O error: timed out> at the invoke method of the DistributedBus. The default of 600 millis on my machine was plenty so I didn't see the issue. The 100 millis in the rust workflow file puts my test right at the edge.

So much trouble for a unit test haha. Anyway, I don't know how to proceed on this one since it looks like there is just not enough time for the request and I don't feel comfortable messing with the workflow setup file. Again if it's more trouble than it's worth I'll take the test out.

autodidaddict commented 3 years ago

I think we're okay pulling out this test and the extra stuff that comes with it. The code change just adds code we know already works in another section. I think we can catch this with some integration tests that we'll hopefully be adding within the next 1-2 milestones.

jose-acevedoflores commented 3 years ago

Removed. Man I feel bad for producing so much chatter for that test. I figured out too late I could just enable the rust.yaml GitHub actions on my fork and test there. I can do a squash commit to clean it up if rebasing is ok with you guys.

autodidaddict commented 3 years ago

Removed. Man I feel bad for producing so much chatter for that test. I figured out too late I could just enable the rust.yaml GitHub actions on my fork and test there. I can do a squash commit to clean it up if rebasing is ok with you guys.

Sounds good to me.