search

film42 / sidekiq-rs

A port of sidekiq to rust using tokio
MIT License
95 stars 10 forks source link

WIP: Rewrite Worker to be generic over Args #3

Closed film42 closed 2 years ago

film42 commented 2 years ago

This is a pretty major change to the lib, but since nobody is likely using it, I think we're ok.

I was hoping to make this lib use generic arguments at the start but couldn't find a way to make it work. The biggest thing I ran into was that I can't create a Box<dyn Worker<T>> without specifying T which made it impossible to build a data structure that could hold any variety of Worker and T. However, through several interesting blog posts and stack overflow comments I realized I could hide the T by wrapping the invocation of the worker with a closure. In other words, because the input and output are the same for any Box<dyn Worker<T>>, the T could be encapsulated inside of a closure. To make things easier I created a WorkerRef type that holds the Fn trait impl and some metadata about the underlying worker. From there I added several layers of Arcs to remove the dyn clone dependency and I think we ended in an OK spot.

One non-standard thing this PR depends on is around deserializing the () type. If you have any valid json value but you want to create a worker that ignores those args, you'll likely get a deserialization error since () doesn't cleanly deserialize from some value. To work around this I guarantee () will always decode by using td::any::TypeId::of::<Args>(). Not the best, but it works.

Finally, by default, to make deserialization convenient, I check for a JsonValue::Array that might be a single element and pluck that element out before deserializing to Args. This is because sidekiq stores args in an array, even if there's only one parameter. In the ruby world, this looks like worker.perform(*args), but this doesn't translate well to rust. This part needs a little more work so that Vec<T> or (T,) workers can be built with this worker.

film42 commented 2 years ago

Ok, I have a work-around that I don't love but is probably good enough for now.

Ideally I could handle this with a negative bound check for Vec<T> or (T,) but since that's not supported, I opted for a hint on the worker itself. Worker trait now implements Worker.disable_argument_coercion(&self) -> bool { false } which can be flipped true if rusty-sidekiq should avoid touching the json value at all. This makes a lot of if/else in lib.rs, but at least it works.