A Cancellable invocable with payload?

andremachado commented 3 years ago

Hello!

Would be nice to be able to have an invocable that's at the same time cancellable and that also contains a payload. What about QueueCancellableInvocableWithPayload()?

Thanks,

André

tolbxela commented 3 years ago

@andremachado: +1 to QueueCancellableInvocableWithPayload

But, you can achieve it also right now with a custom middle class.

Here is my base class with IInvocable, ICancellableTask and IInvocableWithPayload

 public class InvocableBase<T> : ICancellableTask, IInvocable, IInvocableWithPayload<T> {}

mrb0nj commented 3 years ago

@tolbxela I don't suppose you have a slightly more complete example do you? I can't see to get my head around where I need to call QueueInvocableWithPayload/QueueCancellableInvocable and where do i pass in the payload?!

Apologies in advance if i'm being extra stupid today!

tolbxela commented 3 years ago

The base class:

public class InvocableBase<T> : ICancellableTask, IInvocable, IInvocableWithPayload<T>
{
        public CancellationToken Token { get; set; }

        public T Payload { get; set; }

        public virtual Task Invoke()
        {
            return Task.CompletedTask;
        }
}

The task:

public class InvocableTask : InvocableBase<string>
{
        public override async Task Invoke()
        {
              // code
              System.Console.WriteLine(Payload);
        }
}

Using of the InvocableTask:


var TaskGuid = Queue.QueueInvocableWithPayload<InvocableTask , string>("Test");

mrb0nj commented 3 years ago

@tolbxela Thank you for the example, just one question though - and apologies if i'm missing the point massively - where does the CancellationTokenSource instance come from so i can cancel the task later on? Thanks in advance.

tolbxela commented 3 years ago

@mrb0nj: I guess it comes from here:

https://github.com/jamesmh/coravel/blob/1df126935a7cc6a72ae879794edc3aa9164ae2da/Src/Coravel/Queuing/Queue.cs#L59

lengockyquang commented 2 years ago

The base class:

public class InvocableBase<T> : ICancellableTask, IInvocable, IInvocableWithPayload<T>
{
        public CancellationToken Token { get; set; }

        public T Payload { get; set; }

        public virtual Task Invoke()
        {
            return Task.CompletedTask;
        }
}

The task:

public class InvocableTask : InvocableBase<string>
{
        public override async Task Invoke()
        {
              // code
              System.Console.WriteLine(Payload);
        }
}

Using of the InvocableTask:

var TaskGuid = Queue.QueueInvocableWithPayload<InvocableTask , string>("Test");

hi @tolbxela, thanks with your advice, but with your implementation, how can i cancel a specific task that already executed and in processing state

tolbxela commented 2 years ago

@lengockyquang Here is the example in Docs: https://docs.coravel.net/Queuing/#queuing-cancellable-invocables

jamesmh commented 2 years ago

FYI, anyone wanting this - I have some relevant comments in #209 around the mechanics and public interface for this.

TLDR; -> eventually I want to remove QueueCancellableInvocable and just make Coravel automatically "know" based on whether the interface is present on the invocable or not whether a token should be generated for the task. A new method on the queue like TryCancel() would be available instead of returning the token to the caller. See comment on #209. Open to comments etc. 🙂

jamesmh commented 2 years ago

As per the comment above:

Returning the token is a leaky abstraction
There are some potential existing concurrency issues that arise from that method too
There's a loss of modularity in having dedicated methods like QueueCancellableInvocableWithPayload - what if we need that functionality PLUS some other new behaviors? The method names become crazy long, hard to sort through and very rigid.

snargledorf commented 3 months ago

Couldn't an overload of QueueInvocableWithPayload be created that accepts a cancellation token? This way the caller can control cancellation using the standard cancellation pattern.

jamesmh commented 3 months ago

Couldn't an overload of QueueInvocableWithPayload be created that accepts a cancellation token? This way the caller can control cancellation using the standard cancellation pattern.

Since this method comes from an interface, the invocables would have to implement 2 methods now. The caller would not know which method has the correct logic filled/coded out.

Or, the signature of the method could be changed to have an optional/nullable cancellation token param. Considerations that come up from this might include:

Now the caller would have to create a token for every single invocable instance - whether or not it's used.
Do we still use the interface that exists and cause a breaking change?
Do we keep both methods? Would seem odd...

If this option were taken (under consideration) then it would 100% lead to removing the use of the interface to mark the usage of a cancellation token given the points above.

There's a question around philosophy here too: do the ".NET way" of doing things here or "something else that represents 'the coravel way' instead?"

I think there were other choices made (for Coravel) that are worth changing to let's say a more ".NET way" of doing things, but I also think there are some decisions where Coravel deviates a bit from the norm which are nice (which is kinda what sparked Coravel in the first place years ago 😅).

For now, all things worth considering by anyone whos participating in this or other discussions.

Would love to hear what others think on this - does anyone have the same thoughts as before? Any changed thoughts? Indifferent? Something else? 🙂

jamesmh / coravel