1

I've implemented a work queue based on something I found here > Task queue for wp8? ...but am having trouble implementing additional functionality with it.

I'd taken out the Func<Task>'s and replaced them with ICommands (holding their own CancellationTokens), and intended to add Pause(), Resume(), Save() & Restore() methods. This is so OnFormClose() I can pause the queue processing and prompt the user to decide whether he wants to "wait for the queue to finish" (i.e. resume), or "exit now" (i.e. save and exit).

public class WqController
{
    private readonly Queue<ICommand> _queue = new Queue<ICommand>();
    private Task _queueProcessor;
    private ICommand _curCommand;

    public void Enqueue(ICommand command)
    {
        _queue.Enqueue(command);

        if (_queueProcessor == null) _queueProcessor = ProcessQueue();
    }

    private async Task ProcessQueue()
    {
        try
        {
            while (_queue.Count != 0)
            {
                _curCommand = _queue.Peek();

                try
                {
                    await Task.Run(() => _curCommand.Execute());
                }
                catch (OperationCanceledException)
                {
                    Console.WriteLine("QUEUE PAUSED");
                    return;
                }
                catch (Exception)
                {
                    Console.WriteLine("FAILED TO EXECUTE COMMAND");
                }
                _queue.Dequeue();
            }
        }
        finally
        {
            _queueProcessor = null;
            _curCommand = null;
        }
    }

    public async Task Cancel()
    {
        _curCommand.Cts.Cancel(true);
        await _queueProcessor;
    }

    public void Resume()
    {
        _queueProcessor = ProcessQueue();
    }
}

The Save() & Restore() work fine, so I haven't included them here. The Cancel() works intermittently / unreliably, and the Restore() doesn't seem to work at all (confusingly to me, as I'm basically attempting just the same restart as works in the Enqueue() method).

Community
  • 1
  • 1
FrugalTPH
  • 533
  • 2
  • 6
  • 25
  • 1
    It is hard to see how this class holds up any invariants. For example if `Status != WqStatus.Paused` then `Resume` just silently fails. Add asserts making sure that `Resume` is only called when the state is `Paused`. Also, you assume that `_queueProcessor == null`. Add an assert for that as well. And so on. – usr Nov 23 '14 at 21:03
  • I don't *think* that Status or Count are involved in the problem I'm seeing. The resume code is definitely running. Think I'll adjust the code in that respect. – FrugalTPH Nov 23 '14 at 21:43
  • When I break on `Resume()`, `_queueProcessor`'s status says "WaitingForActivation", if that is any help? – FrugalTPH Nov 23 '14 at 21:49
  • 1
    It seems to me we are missing code to determine what goes on. For example, you talk about cancellation, but where do really check your `CancellationToken` inside the code? i don't see that part. – Yuval Itzchakov Nov 24 '14 at 07:31
  • @YuvalItzchakov: I'm doing a `_curCommand.Cts.Token.ThrowIfCancellationRequested()` immediately before any long running task in any code called by the commands going through this queue. Your comment has just helped me maybe explain the intermittentness of `Cancel()`. If cancel is called after the last cancellation check has passed, then the exception isn't thrown, and the next command gets loaded (along with a new cancellation token). I've therefore added `if (_curCommand.Cts.Token.IsCancellationRequested) return;` immediately after `_queue.Dequeue`. – FrugalTPH Nov 24 '14 at 11:09

1 Answers1

0

I got this working and thought I should outline my solution here.

It turns out my use of cancellation tokens was a little haphazard, which was preventing this class from functioning as intended. For example, the following issues were relevant:

  1. If Cancel was called after the last cancellation check had passed in the command, a new command would be loaded (along with its own new cancellation token), and as such the cancel call would have been lost / ignored. This was solved with if (_curCommand.Cts.Token.IsCancellationRequested) return; right after _queue.Dequeue();.

  2. After a cancel had been called, if the command were to be resumed later, then it would need a new cancellation token (otherwise the existing one with cancel = true would still be active). The line _curCommand.InvalidateCancellationToken(); does this by setting the token to null, and then my command refreshes the token when next called.

The full code I used:

public class WqController
{
    private readonly Queue<ICommand> _queue = new Queue<ICommand>();
    private Task _queueProcessor;
    private ICommand _curCommand;

    public void Enqueue(ICommand command)
    {
        _queue.Enqueue(command);

        if (_queueProcessor == null) _queueProcessor = ProcessQueue();
    }

    private async Task ProcessQueue()
    {
        try
        {
            while (_queue.Count != 0)
            {
                _curCommand = _queue.Peek();

                try
                {
                    await Task.Run(() => _curCommand.Execute());
                }
                catch (OperationCanceledException)
                {
                    _curCommand.InvalidateCancellationToken();
                    Console.WriteLine("QUEUE PAUSED");
                    return;
                }
                catch (Exception)
                {
                    Console.WriteLine("FAILED TO EXECUTE COMMAND");
                }
                _queue.Dequeue();
                if (_curCommand.Cts.Token.IsCancellationRequested) return;
            }
        }
        finally
        {
            _queueProcessor = null;
            _curCommand = null;
        }
    }

    public async Task Cancel()
    {
        _curCommand.Cts.Cancel(true);
        await _queueProcessor;
    }

    public void Resume()
    {
        _queueProcessor = ProcessQueue();
    }
}

This all seems to work very smoothly now, and is a big improvement on the Background worker queue implementation I had been using prior.

FrugalTPH
  • 533
  • 2
  • 6
  • 25