Refactor TriggerExecutor to have associated types for instances
#2366
Conversation
alexcrichton
force-pushed
the
triggers-custom-instances
branch
2 times, most recently
from
3613467
to
ea0e02b
Compare
|
Is the proposal that every custom trigger should have to copy the stuff that is added to the timer sample? I'm not enthusiastic about it if so. I'd like the direction to be helping trigger authors focus on the trigger rather than to making them wrangle wasmtime internals. @lann I don't think this will affect service chaining, at least as it stands today. The current implementation re-enters at |
examples/spin-timer/src/lib.rs
Outdated
| @@ -113,15 +118,30 @@ impl TriggerExecutor for TimerTrigger { | |||
| } | |||
| Ok(()) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Per my comment on the issue, let's find a way that doesn't require every custom trigger to copy this code. (That might be as simple as defaulting things in the trait, I don't know.)
There was a problem hiding this comment.
Sadly this can't be defaulted as-is because you can't have associated type defaults on traits (rust-lang/rust#29661).
There was a problem hiding this comment.
@alexcrichton Would it be reasonable to add traits e.g. spin_core::SpinInstance(Pre) and have these methods return impl SpinInstance(Pre)?
There was a problem hiding this comment.
I suppose the SpinInstance trait might be a bit gnarly there and could end up boiling down to a component-or-module enum in disguise. Depends on what exactly you're hoping to enable here.
There was a problem hiding this comment.
Sounds like a reasonable concern to me! I ended up creating a TriggerInstancePre trait for the spin-trigger trait before I came here and read @lann's comments. I think that this should have the desired ergonomics but I'm happy to move/rename the trait too
There was a problem hiding this comment.
I think given the amount of cargo-culty copying required to make a custom trigger today, this is fine. The whole interaction between spin-core, spin-app, and component instantiation is pretty wonky and became wonkier with the (partial) migration to components, and I don't think this trigger architecture can or should survive long-term.
Probably more important in the near-term would be to actually document (mea culpa) this interface a bit to help people along.
There was a problem hiding this comment.
If I read this correctly, trigger authors would no longer cargo-cult the let EitherInstance::Component... fragment and instead cargo-cult the type InstancePre = ... fragment. That feels like less cargo-culting and I'm in favour. Thank you Alex for simplifying this!
(If I've misread it, though, @alexcrichton, please hold off so I can get my head around the actual experience.)
There was a problem hiding this comment.
No yep you've read correctly, and once Rust supports defaults for associated types we can remove the type InstancePre = ... as well (or at least that's my understanding of associated typ defaults)
Nope, I just knew you've been iterating on that implementation and wanted to make sure you were in the loop here. |
alexcrichton
force-pushed
the
triggers-custom-instances
branch
2 times, most recently
from
f203ef5
to
fa754ad
Compare
Moves the `EitherInstance*` logic into just the `spin-trigger-http` crate where the redis trigger, for example, only has to deal with components. The intention of this is to open up future customization of instances within a trigger that don't necessarily need to affect all other triggers. The new associated type is bound itself by a new trait `TriggerInstancePre` which encapsulates the required functionality. This trait has a default implementation for `spin_core::InstancePre<T>` which serves for triggers that only work with components. Signed-off-by: Alex Crichton <alex@alexcrichton.com>
alexcrichton
force-pushed
the
triggers-custom-instances
branch
from
fa754ad
to
9562a71
Compare
Moves the
EitherInstance*logic into just thespin-trigger-httpcrate where the redis trigger, for example, only has to deal with components. The intention of this is to open up future customization of instances within a trigger that don't necessarily need to affect all other triggers.