coravel icon indicating copy to clipboard operation
coravel copied to clipboard

A Cancellable invocable with payload?

Open andremachado opened this issue 4 years ago • 11 comments

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é

andremachado avatar Jul 30 '20 01:07 andremachado

@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> {}

tolbxela avatar Oct 14 '20 14:10 tolbxela

@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!

mrb0nj avatar Dec 31 '20 10:12 mrb0nj

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");

tolbxela avatar Dec 31 '20 17:12 tolbxela

@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.

mrb0nj avatar Jan 01 '21 19:01 mrb0nj

@mrb0nj: I guess it comes from here:

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

tolbxela avatar Jan 01 '21 20:01 tolbxela

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

lengockyquang avatar Jan 03 '22 06:01 lengockyquang

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

tolbxela avatar Jan 05 '22 11:01 tolbxela

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 avatar Jan 05 '22 13:01 jamesmh

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.

jamesmh avatar Jan 05 '22 13:01 jamesmh

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.

snargledorf avatar Feb 08 '24 16:02 snargledorf

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 avatar Feb 08 '24 19:02 jamesmh