Custom slot suppliers #343
Conversation
|
|
||
| fn build_tuner(options: Struct) -> Result<TunerHolder, Error> { | ||
| fn extract_poller_behavior(poller_behavior: Struct) -> Result<PollerBehavior, Error> { | ||
| Ok(if poller_behavior.member::<usize>(id!("initial")).is_ok() { |
There was a problem hiding this comment.
I feel like we should use some explicit discrimination tag or something for these. More than one issue came up in Python because of this kind of implicit discrimination. EX: A new variant comes along later that also has an initial field and all of a sudden things go wrong.
I won't block on it but IMO it's worth doing the small amount of legwork for the safety guarantee
There was a problem hiding this comment.
This is your code from #275 that I just moved from below build_tuner to above
There was a problem hiding this comment.
Haha fair enough, I think I've learned more now about how I don't love this, now. Might be good to improve it
| loop { | ||
| match self.attempt_reserve_slot(ctx.clone()).await { | ||
| Ok(permit) => { | ||
| return SlotSupplierPermit::with_user_data(Arc::new(permit)); |
There was a problem hiding this comment.
It would make sense, if reasonable, for the underlying magnus object to be some kind of clonable ref rather than Arc here, as there semantically is only one "permit", and this way we can't accidentally dupe it Rust side. Not really hugely important but IIRC in Python we are cloning the py object ref rather than the permit itself when we need to hand it back and forth.
There was a problem hiding this comment.
You can't accidentally dupe Rust side because you don't accept it as cloneable. The actual Ruby object is not cloneable, it has a single reference (https://docs.rs/magnus/latest/magnus/value/struct.BoxValue.html). I have to clone this because I have to extract the value from a Ruby context in another thread later. So I need some kind of send/sync/clone holder that will drop the value when the last holder goes away. I think Arc fits this.
There was a problem hiding this comment.
I made a https://github.com/temporalio/sdk-core/blob/c1060e5cdbe9b456db922f154b37f22ede14cd7f/core/src/abstractions/take_cell.rs#L7 which maybe does what you need, but, Arc is fine too then
| let (result_tx, mut result_rx) = unbounded_channel(); | ||
| let (cancel_proc_ready_tx, mut cancel_proc_ready_rx) = unbounded_channel(); |
There was a problem hiding this comment.
Ideally these would be oneshot channels which would probably be more accurate semantics. In principle you can have just one actually, and use a Result | CancelReady enum too I think.
In practice you have these closures that need to own them separately so I can see why that didn't really work out. From my reading it doesn't look like you can change them to be not 'static, if possible that would solve it.
There was a problem hiding this comment.
Ideally these would be oneshot channels which would probably be more accurate semantic
Not exactly. The oneshot sender does not allow you to clone it and it consumes it on send, but the result may get an error on Ruby response or Ruby callback (or technically both, but we only care about the first). Ruby callbacks where these are called from are "procs" passed to the user, and even though we know they are FnOnce, to Ruby they are actually Fn and technically users could call them multiple times.
In principle you can have just one actually, and use a Result | CancelReady enum too I think
No, cancel ready is sent at a different time and is an out of band thing (sometimes before response, sometimes after)
| &self, | ||
| method: &'static LazyId, | ||
| ctx: impl TryIntoValue + Send + 'static, | ||
| on_success: impl Fn(&Ruby, Value) + Send + 'static, |
There was a problem hiding this comment.
The semantics of this are FnOnce, which would allow you to avoid the whole option dance later. It's hard for me just reading to see if there's some obvious reason that doesn't work, but we should use it if it does.
There was a problem hiding this comment.
This is moved into an FnMut as part of https://docs.rs/magnus/latest/magnus/struct.Ruby.html#method.proc_from_fn. These are Ruby procs that we want to only be called once, but from a system POV technically they could be called multiple times. It was clearer to make it harmless to call multiple times than it was to try to move this FnOnce into an FnMut by wrapping it in some kind of thing.
| ruby.get_inner(&SLOT_INFO_WORKFLOW_CLASS).funcall( | ||
| id!("new"), | ||
| ( | ||
| // Workflow type |
There was a problem hiding this comment.
These comments seem unnecessary
There was a problem hiding this comment.
For every parameter I am using from these holder objects to tie to Ruby parameters, I clarify the Ruby parameter name it maps to since the class/method is defined in a far away place. I am just being consistent with myself here, even if some of them are redundant.
…upplier # Conflicts: # temporalio/Cargo.lock
What was changed
Temporalio::Worker::Tuner::SlotSupplier::Custominstances for slot suppliersChecklist