Add Queue.QueueCancellableInvocableWithPayload method

[mrb0nj]

Implements both ICancellableTask and IInvocableWithPayload<T> for Issue #182

[jamesmh]

Thanks! I'm going to be away for a few weeks most likely, just FYI. Will check this out when things settle down for me 👍

[achmstein]

Hi,

I cannot find QueueCancellableInovcableWithPayload method in Coravel package.

Can you help me?

[lengockyquang]

Waiting for this pull request merge !

[jamesmh]

Thanks again for this. I have some minor suggestions :)

What if instead of returning the CancellationToken to the caller, we would add a new method on the IQueue like void Cancel(string uniqueIdentifier). Since the queue does "stuff" like cancel tokens and removes them from the collection of "tracked" tokens, returning a token to the caller is much like a leaky abstraction. It could potentially also lead to unexpected issues like:

1. Caller gets the token back
2. Queue runs, disposes the token
3. Caller tries to cancel but gets an exception.
4. Even if the caller wraps their cancellation calls with a "check", the queue, concurrently, could still dispose that token, leading to intermittent exceptions even if the caller is doing "proper" checks.

So I think it best to keep cancellation logic internal, so that we have more control over the concurrency, etc. of these things.

If the token exists and is not cancelled, then cancel it. Like this existing method does. You won't have to remove the token since that logic already exists whenever the jobs are invoked. By using the concurrent dict you'll just have to make sure you use this method to get access to the token (otherwise, if you used ContainsKey() and then tried to fetch the value via an index method you might get a concurrency exception. The item could be removed by another thread just after the call to ContainsKey() but before you fetch the value. TryGetValue() makes sure you just do the "check" and "fetch" atomically.)

Also, internally it's possible to have the same concurrency issue as mentioned at the beginning of my comment. It might be worth catching ObjectDisposedException as mentioned here.

What do you think? Did I miss something? Does that make sense?

[jamesmh]

More on the comment above, eventually I actually think removing QueueCancellableInvocable might make sense.

In the other queue methods, we would simply check if the invocable in question implements ICancellableTask.

Doing this right now might make sense - don't create a new method QueueCancellableInvocableWithPayload, but check within QueueInvocableWithPayload if the task implements ICancellableTask and add a new token internally + the new method mentioned in the comment above. In fact, doing this for all existing "queue" methods now would work well, and then I'll look at setting up a long-term schedule to depreciate the other methods as part of a major release, etc.

The existing logic, I believe, is susceptible right now to the ObjectDisposedException... so that is just one other reason why keeping all this logic internal has a benefit (e.g. we can handle that exception internally and the caller doesn't need to know about how to use tokens. The UX becomes way simpler for the caller).