2
\$\begingroup\$

My use case is this:

  1. There is a long running process that I want to ensure is only ever running at most only a single time.
  2. Multiple consumers from different threads may wait on the calculation.
  3. If the process is not running, the first consumer triggers it.

Here is the code:

public class Worker {
    private int _working;
    private TaskCompletionSource? _tcs;

    public async Task DoWorkAsync() {
        var triggerWork = false;
        if (Interlocked.CompareExchange(ref _working, 1, 0) == 0) {
            triggerWork = true;
            _tcs = new TaskCompletionSource();
        }

        if (!triggerWork) {
            await _tcs!.Task;

            /*
             * Point of interest: previous worker might have just finished,
             * and a new worker might have started - but that is ok because
             * "DoWorkInternalAsync" is a transactioned operation.
             */

            return;
        }

        await DoWorkInternalAsync();
        _tcs!.SetResult();

        _working = 0;
    }

    private async Task DoWorkInternalAsync() {
        // Do work
    }
}

I would like a review for my code for efficiency and correctness, and also I'd be happy to hear suggestions on how to do it more succinctly (perhaps with the use of a library or some BCL class I might have missed)

Thanks

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Your code looks ok with respect to correctness, but I have not tested it. One option to make it shorter is to cache the task directly. As you have Task (and not ValueTask) you may await it many times.

So more like this:

public class Worker
{
    private object _lock = new();
    private Task<int>? _task;

    public Task<int> DoWorkAsync()
    {
        return _task ?? SetTaskIfNull();

        Task<int> SetTaskIfNull()
        {
            lock (_lock)
                return _task ??= DoWorkInternalAsync();
        }
    }

    private async Task<int> DoWorkInternalAsync()
    {
        await Console.Out.WriteLineAsync("Start calc");
        await Task.Delay(1000);
        await Console.Out.WriteLineAsync("Calc done");
        return 10;
    }
}

You do not need to await on the child operation when you only want to return the task. This means you can use the lock statement and the code will be easier to read. The first null check avoid the lock, if the task is already there. The second null check prevents duplicate creation.

So it appears that you want to restart the calculation in case that there is only a completed one?

Then I'd suggest this:

public class Worker
{
    private object _lock = new();
    private Task<int>? _task;

    public Task<int> DoWorkAsync()
    {
        lock(_lock)
        {
            // null would mean that this thread is the first
            // true means we need to restart
            if ((_task?.IsCompleted) != false)
                _task = DoWorkInternalAsync(1);

            return _task;
        }
    }

    private async Task<int> DoWorkInternalAsync(int v)
    {
        await Console.Out.WriteLineAsync("Start calc");
        await Task.Delay(1000);
        await Console.Out.WriteLineAsync("Calc done");
        return v;
    }
}
\$\endgroup\$
6
  • \$\begingroup\$ If I'm not mistaken, this is incorrect: if two threads discover that _task is null and enter SetTask, they will each get a different task and the internal calculation will be performed twice. \$\endgroup\$
    – Aviad P.
    Jul 15, 2023 at 13:33
  • \$\begingroup\$ That's where the lock comes in. Only one Thread can enter the lock, so the first will see the null, assign _task and return it. Then the second will see it is not null and return it also. IN my test I always had exactly once execution. \$\endgroup\$ Jul 15, 2023 at 13:35
  • \$\begingroup\$ Two threads can to into SetTaskIfNull at the same time, both will create a new task \$\endgroup\$
    – Aviad P.
    Jul 15, 2023 at 14:00
  • \$\begingroup\$ You second variation is not good either - it will only ever calculate once. Will not refresh the calculation if called down the line \$\endgroup\$
    – Aviad P.
    Jul 15, 2023 at 14:02
  • \$\begingroup\$ I have edited my answer with a version that triggers a recalculation when no operation is running. Still, the lock statement will ensure that only one thread at a time can enter the lock and thus only one thread will actually create the Task. \$\endgroup\$ Jul 15, 2023 at 14:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.