Custom slot suplier #372

Sushisource · 2024-11-25T22:08:15Z

What was changed

Add user-implementable slot suppliers

Why?

Parity. Cool feature.

Checklist

Closes
How was this tested:
Added tests
Any docs updates needed?
Will need to follow up with docs for custom interface in all langs

Sushisource · 2024-12-04T00:27:31Z

Let's merge #376 first then I will rebase

src/Temporalio/Bridge/CustomSlotSupplier.cs

cretz · 2024-12-04T14:42:48Z

src/Temporalio/Bridge/CustomSlotSupplier.cs

+                        cancelTokenSrc.Dispose();
+                        return;
+                    }
+                    catch (OperationCanceledException)


Suggested change

catch (OperationCanceledException)

catch (OperationCanceledException) when (cancelTokenSrc.IsCancellationRequested)

I assume there will be a missed reservation if a user raised this exception for whatever reason unrelated to our cancellation. In fact, should we either 1) always call complete_async_reserve before returning, even on cancel, and/or 2) always avoid calling complete_async_reserve if cancellation is requested (user can choose to ignore cancellation token)

It won't matter - because if the token is cancelled Rust has already given up waiting for anything to come back through the completion channel, so if something is sent or not, either way it's ignored.

I am saying OperationCanceledException can be thrown from ReserveSlotAsync without it being the result of this cancellation token, hence the when suggestion here.

But in addition to that, remember that Box::into_raw(Box::new(tx)) means that Rust will never drop the tx side of the oneshot until it's Box::from_raw'd in complete_async_reserve. So to not have memory leaks, you probably need to call complete_async_reserve even on cancel, but just give it a user permit of 0 or something.

Ah, yeah I get what you mean now

src/Temporalio/Bridge/CustomSlotSupplier.cs

cretz · 2024-12-04T14:49:17Z

src/Temporalio/Bridge/CustomSlotSupplier.cs

+            }
+        }
+
+        private unsafe void Reserve(Interop.SlotReserveCtx ctx, void* sender)


Suggested change

private unsafe void Reserve(Interop.SlotReserveCtx ctx, void* sender)

private unsafe void Reserve(Interop.SlotReserveCtx* ctx, void* sender)

I would have expected these all to be pointers so they aren't copied (so *const SlotReserveCtx on the Rust side). I think it's clearer that these short-lived Rust-owned things be passed by reference to Rust, so you don't need to re-capture a fixed pointer later.

Yeah, I was thinking about doing that, makes sense.

cretz · 2024-12-04T14:52:44Z

src/Temporalio/Bridge/CustomSlotSupplier.cs

+            {
+                try
+                {
+                    var handle = GCHandle.Alloc(cancelTokenSrc);


Is this only freed if CancelReserve is called? Consider inlining the logic of this method with Reserve and freeing this in a finally. Once the try/catch is removed and the need for a fixed pointer is removed (see other comments), it's a short method anyways.

EDIT: Looking at Rust code, it appears CancelReserve is always called even if cancellation doesn't need to occur. Unsure if it's worth adding that comment somewhere on the .NET side. Regardless, so this is technically fine to free inside of callback, though there is still probably clarity in inlining this method.

Added comment

cretz · 2024-12-04T15:21:22Z

src/Temporalio/Worker/Tuning/SlotReserveContext.cs

+        /// <param name="ctx">The bridge version of the slot reserve context.</param>
+        internal SlotReserveContext(Temporalio.Bridge.Interop.SlotReserveCtx ctx)
+        {
+            this.SlotType = ctx.slot_type switch


A bit surprised a switch expression works in older .NET (or maybe it doesn't, didn't check CI)

cretz · 2024-12-04T15:24:57Z

src/Temporalio/Worker/Tuning/ICustomSlotSupplier.cs

+    /// <remarks>
+    /// WARNING: Custom slot suppliers are currently experimental.
+    /// </remarks>
+    public interface ICustomSlotSupplier : ISlotSupplier


Could be discussed whether this is better as an abstract class, but I am fine with this too

cretz · 2024-12-04T15:25:07Z

src/Temporalio/Worker/Tuning/ICustomSlotSupplier.cs

+        ///  to cancel the operation.</param>
+        /// <returns>A permit to use the slot which may be populated with your own data.</returns>
+        /// <exception cref="OperationCanceledException">Cancellation requested.</exception>
+        public Task<ISlotPermit> ReserveSlotAsync(SlotReserveContext ctx, CancellationToken cancellationToken);


Suggested change

public Task<ISlotPermit> ReserveSlotAsync(SlotReserveContext ctx, CancellationToken cancellationToken);

Task<ISlotPermit> ReserveSlotAsync(SlotReserveContext ctx, CancellationToken cancellationToken);

Implied

cretz · 2024-12-04T15:26:29Z

src/Temporalio/Worker/Tuning/ISlotPermit.cs

Arguably this could just not exist and we could accept any object from the user. Alternatively we could just have a sealed SlotPermit that has user data inside it like Java has. I think either would be better than this, because with this there is no easy/default implementation for people who don't care about user data.

Yeah, good point about not being able to create something easily. I will go with the Java style way because I don't like the idea of just returning any old object if for no other reason than that it's nice to know what is a permit and what isn't.

There was some reason I had to change this to an interface (it was a class before) that I've forgotten now and I'm sure is going to bite me again, but, I'm sure it's not blocking.

cretz · 2024-12-04T15:29:25Z

src/Temporalio/Worker/Tuning/ISlotSupplier.cs

+    /// <see cref="FixedSizeSlotSupplier"/> and <see cref="ResourceBasedSlotSupplier"/>.
+    ///
+    /// In order to implement your own slot supplier, you can implement the
+    /// <see cref="ICustomSlotSupplier"/> interface.
    /// </summary>
    public interface ISlotSupplier


If you do change ICustomSlotSupplier to an abstract class, this could also be an abstract class with an internal constructor thereby making it where people can't accidentally provide an invalid instance.

This one doesn't work so much because the built-in impls are records and thus can't extend from a class, only interfaces.

We could change them to classes, but that's technically breaking, though that's fine at this point and I think is probably worth it.

👍 I don't have a strong opinion. I'm ok leaving this as an interface or doing the compat break on the fixed/resource suppliers.

Everything working except cancelling reserves

cretz

LGTM, only minor things

cretz · 2024-12-05T13:06:45Z

src/Temporalio/Bridge/CustomSlotSupplier.cs

+                    {
+                        try
+                        {
+                            ConfiguredTaskAwaitable<Temporalio.Worker.Tuning.SlotPermit> reserveTask;


Never seen this type used. Docs say it's an internal compiler thing. I think it should be Task<.

It won't compile as just Task, but I can move the configure await to where it's awaited

Yeah, assuming it's because of the unsafe block, I'd say something like this should work:

SlotReserveContext ctx; unsafe { ctx = ReserveCtxFromBridge((Interop.SlotReserveCtx*)ctx.ToPointer()); } var permit = await userSupplier.ReserveSlotAsync(ctx, cancelTokenSrc.Token).ConfigureAwait(false);

cretz · 2024-12-05T13:11:03Z

src/Temporalio/Bridge/CustomSlotSupplier.cs

+        private unsafe void CancelReserve(void* tokenSrc)
+        {
+            var handle = GCHandle.FromIntPtr(new IntPtr(tokenSrc));
+            var cancelTokenSrc = (CancellationTokenSource)handle.Target!;


To confirm, this can never be called before set_reserve_cancel_target returns and never called after complete_async_reserve returns right?

It shouldn't be possible, no

cretz · 2024-12-05T13:12:55Z

src/Temporalio/Bridge/OptionsExtensions.cs

            else
            {
-                throw new ArgumentException(
-                    "ISlotSupplier must be one of the types provided by the library");
+                throw new ArgumentException("ISlotSupplier is an invalid type");


Suggested change

throw new ArgumentException("ISlotSupplier is an invalid type");

throw new ArgumentException("Unknown slot supplier");

cretz · 2024-12-05T13:17:12Z

src/Temporalio/Worker/Tuning/SlotPermit.cs

+namespace Temporalio.Worker.Tuning
+{
+    /// <summary>
+    /// A permit to use a slot for a workflow/activity/local activity task.


Can tell users they can extend this class if they want (unlike what we allowed in Java)

cretz · 2024-12-05T13:17:42Z

src/Temporalio/Worker/Tuning/SlotPermit.cs

+        public SlotPermit()
+        {
+            UserData = null;
+        }


Suggested change

public SlotPermit()

{

UserData = null;

}

public SlotPermit() => UserData = null;

Just an FYI, lots of these single-line methods can be shortened. But this is pedantic/unimportant.

cretz · 2024-12-05T13:18:00Z

src/Temporalio/Worker/Tuning/SlotReserveContext.cs

+        bool IsSticky)
+    {
+    }


Suggested change

bool IsSticky)

{

}

bool IsSticky);

cretz · 2024-12-05T13:24:04Z

src/Temporalio/Worker/Tuning/CustomSlotSupplier.cs

+    /// <remarks>
+    /// WARNING: Custom slot suppliers are currently experimental.
+    /// </remarks>
+    public abstract class CustomSlotSupplier : SlotSupplier


I wonder if you have to have a public, parameterless constructor here for accessibility outside the project. The tests package doesn't do a good job of testing this because we set System.Runtime.CompilerServices.InternalsVisibleTo to the test project so it can access some visible-to-testing only things (I forget what). May need just a quick confirmation this can be extended outside the project. Can just be some scratch code in tests/Temporalio.SmokeTests, doesn't have to be committed.

It appears to work just fine without it. The bench project is easier to use since it doesn't require nupkg mumbojumbo but also doesn't do internals visible.

Sushisource force-pushed the custom-slot-suplier branch 9 times, most recently from 8f822c1 to 0a9de76 Compare November 28, 2024 01:44

Sushisource force-pushed the custom-slot-suplier branch 5 times, most recently from ff2f074 to 23d874b Compare December 3, 2024 23:00

Sushisource marked this pull request as ready for review December 3, 2024 23:00

Sushisource changed the title ~~[Draft] Custom slot suplier~~ Custom slot suplier Dec 3, 2024

Sushisource mentioned this pull request Dec 4, 2024

Update Core #376

Merged

cretz reviewed Dec 4, 2024

View reviewed changes

Sushisource added 6 commits December 4, 2024 09:31

Initial scaffolding

61939fb

Get bridge data passed through

4bf4438

Callbacks working / test passes

c9f1315

Expanded tests

f735b23

Everything working except cancelling reserves

Cancellation Tokens

0ee6a32

No need to make gchandles for permits at all

478324f

Sushisource force-pushed the custom-slot-suplier branch 5 times, most recently from 4e356fc to f97d26c Compare December 4, 2024 19:15

Sushisource force-pushed the custom-slot-suplier branch from f97d26c to 008aa55 Compare December 4, 2024 19:32

Review fixes

14c12af

Sushisource force-pushed the custom-slot-suplier branch from 008aa55 to 14c12af Compare December 4, 2024 22:31

cretz reviewed Dec 5, 2024

View reviewed changes

Last review bits

2c8707b

cretz approved these changes Dec 5, 2024

View reviewed changes

Sushisource merged commit 15a3142 into main Dec 5, 2024
8 checks passed

Sushisource deleted the custom-slot-suplier branch December 5, 2024 22:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom slot suplier #372

Custom slot suplier #372

Sushisource commented Nov 25, 2024 •

edited

Loading

Sushisource commented Dec 4, 2024

cretz Dec 4, 2024

Sushisource Dec 4, 2024

cretz Dec 4, 2024

Sushisource Dec 4, 2024

cretz Dec 4, 2024

Sushisource Dec 4, 2024

cretz Dec 4, 2024

Sushisource Dec 4, 2024

cretz Dec 4, 2024

cretz Dec 4, 2024

cretz Dec 4, 2024

cretz Dec 4, 2024

Sushisource Dec 4, 2024 •

edited

Loading

cretz Dec 4, 2024

Sushisource Dec 4, 2024 •

edited

Loading

cretz Dec 4, 2024

cretz left a comment

cretz Dec 5, 2024

Sushisource Dec 5, 2024 •

edited

Loading

cretz Dec 5, 2024 •

edited

Loading

cretz Dec 5, 2024

Sushisource Dec 5, 2024

cretz Dec 5, 2024

cretz Dec 5, 2024

cretz Dec 5, 2024

cretz Dec 5, 2024

cretz Dec 5, 2024

Sushisource Dec 5, 2024

	catch (OperationCanceledException)
	catch (OperationCanceledException) when (cancelTokenSrc.IsCancellationRequested)

	private unsafe void Reserve(Interop.SlotReserveCtx ctx, void* sender)
	private unsafe void Reserve(Interop.SlotReserveCtx* ctx, void* sender)

	public Task<ISlotPermit> ReserveSlotAsync(SlotReserveContext ctx, CancellationToken cancellationToken);
	Task<ISlotPermit> ReserveSlotAsync(SlotReserveContext ctx, CancellationToken cancellationToken);

	throw new ArgumentException("ISlotSupplier is an invalid type");
	throw new ArgumentException("Unknown slot supplier");

Custom slot suplier #372

Custom slot suplier #372

Conversation

Sushisource commented Nov 25, 2024 • edited Loading

What was changed

Why?

Checklist

Sushisource commented Dec 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sushisource Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sushisource Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sushisource Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

cretz Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sushisource commented Nov 25, 2024 •

edited

Loading

Sushisource Dec 4, 2024 •

edited

Loading

Sushisource Dec 4, 2024 •

edited

Loading

Sushisource Dec 5, 2024 •

edited

Loading

cretz Dec 5, 2024 •

edited

Loading