Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A Cancellable invocable with payload? #182

Open
andremachado opened this issue Jul 30, 2020 · 11 comments
Open

A Cancellable invocable with payload? #182

andremachado opened this issue Jul 30, 2020 · 11 comments

Comments

@andremachado
Copy link

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
Copy link
Contributor

tolbxela commented Oct 14, 2020

@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
Copy link

mrb0nj commented Dec 31, 2020

@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
Copy link
Contributor

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
Copy link

mrb0nj commented Jan 1, 2021

@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
Copy link
Contributor

tolbxela commented Jan 1, 2021

@mrb0nj: I guess it comes from here:

var tokenSource = new CancellationTokenSource();

@lengockyquang
Copy link

lengockyquang commented Jan 3, 2022

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
Copy link
Contributor

tolbxela commented Jan 5, 2022

@jamesmh
Copy link
Owner

jamesmh commented Jan 5, 2022

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
Copy link
Owner

jamesmh commented Jan 5, 2022

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
Copy link

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
Copy link
Owner

jamesmh commented Feb 8, 2024

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? 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants