simplify break condition for epoch ticker thread

[benbrandt]

It seems that the crossbeam channel was just being used as a way to exit the ticker loop. Once the sender was dropped, then try_recv would fail and the loop would break.

However, it also seems, based on the wasmtime docs that just upgrading a weak reference to the Arc should serve roughly the same purpose. Once all references to the Engine are dropped, then the loop should break.

If there was a reason to use a channel here because there are potential references to the Engine Arc I didn't catch in the codebase, then feel free to ignore.

Thanks!

[lann]

However, it also seems, based on the wasmtime docs that just upgrading a weak reference to the Arc should serve roughly the same purpose. Once all references to the Engine are dropped, then the loop should break.

Seems a little fragile to me since any consumer could clone the Engine, but given the low impact of leaking this thread its probably fine. 🤷

In any case I certainly can't remember why I pulled in crossbeam-channel for this. 🤔

[benbrandt]

@lann this was also my thought... It does seem fragile, but should the ticker keep going as long as there is potentially something using the engine?

Open to reasons why this might be a bad idea, not sure how often the engine gets cloned. I mostly opened this as a learning exercise. If it simplified things, great. If it introduces an issue, I'm curious to know why.

[lann]

Oh sorry, I should have added: I'm fine with merging this. 🙂 I saw that @rylev added himself as a reviewer and want to give him a chance to weigh in.

[benbrandt]

@lann all good, I think I caught that 😄

Another option I guess would be to store the handle to the thread and make sure it shuts down in a drop handler or something?

[lann]

After reviewing how all of this is supposed to work I think this PR actually makes this crate safer to use: If a consumer of this crate wanted to shut down the engine (but not the entire process) while a guest instance was running, killing the epoch ticker thread could leave a misbehaving guest running indefinitely where it would normally time out. By tying the thread lifetime to the lifetime of the wasmtime::Engine that should become impossible.

I don't think this is relevant to any of the actual Spin trigger handlers (which always terminate immediately after dropping the engine anyway), but on balance I think this is an improvement.

[lann]

(I'll still wait for @rylev to take a look but I'm more confident this should merge)

[rylev]

I was also initially worried about this being fragile, but on further thinking, this seems to be exactly the behavior we want. Thanks so much 🎉

[benbrandt]

@lann and @rylev thanks for the review!

Thinking on it more, it might feel less fragile if this thread was owned by the same struct as the actual engine, inside the Arc, so it could be cleaned up on Drop. But this would have to be in wasmtime itself I guess and since it isn't always used in this mode might also not be the right place for it.

But I appreciate you all taking time to validate the idea, glad it helps!