Bug in web example

[qrjo]

There's a bug in RequestTimeConsumer in your web example (link).

The class inherits from BackgroundService, but does so in a wrong way: the ExecuteAsync implementation returns a completed task, while the thread with the consumer loop is still running. The task returned from ExecuteAsync should not complete until the entire background process is finished. The way it's done now, Dispose may be called before the consumer loop has stopped running, which may cause errors.

I also have a (somewhat) related question: why is a thread used here, rather than a task?

[amirvalhalla]

Hi @qrjo

in the web example , in ExecuteAsync used thread because after new thread

return Task.CompleteTask;

so if you use task instead of Thread here , the code just will run one time and task will finish after first run , if you wanna use task instead of Thread you should delete

return Task.CompleteTask;

also using Task instead of Thread has better performance because Thread will use more resources than Task

[qrjo]

Could just use Task.Run(() => StartConsumerLoop(stoppingToken)) instead of new Thread(() => StartConsumerLoop(stoppingToken)).Start().

But returning Task.CompleteTask from ExecuteAsync is wrong whether you use threads or tasks.

It would be better to do something like this:

protected override Task ExecuteAsync(CancellationToken stoppingToken)
{
    Thread t = new Thread(() => StartConsumerLoop(stoppingToken));
    t.Start();

    return Task.Run(() => t.Join());
}

or

protected override Task ExecuteAsync(CancellationToken stoppingToken)
{
    return Task.Run(() => StartConsumerLoop(stoppingToken), stoppingToken);
}

[amirvalhalla]

Ok , but it's just an example so depends on your business you can implement your consumer by different approaches

[qrjo]

Yeah, but a broken example isn't a good example, right?

[amirvalhalla]

in some aspects I agree but in some others not , but I think the best solution for this issue is adding multiple examples instead of one

[mhowlett]

i might have merged https://github.com/confluentinc/confluent-kafka-dotnet/pull/1446 too quickly. maybe we just revert that.

[anchitj]

Closing, as the suggested changes are merged.