rabbitmq / rabbitmq-dotnet-client

RabbitMQ .NET client for .NET Standard 2.0+ and .NET 4.6.2+
https://www.rabbitmq.com/dotnet.html
Other
2.06k stars 574 forks source link

If CreateConnection is called on a thread with a non-default TaskScheduler it will attempt to start the MainLoop on that scheduler. #1355

Closed michac closed 9 months ago

michac commented 1 year ago

Describe the bug

CreateConnection will timeout if it's called on a thread with a non-default TaskScheduler that is blocked by the call to CreateConnection, for example a single threaded message pump.

Reproduction steps

I apologize for the use of the Akka library, which may make the example less clear. It is only there to demonstrate how CreateConnection behaves on a thread with the TaskScheduler overridden. I'm sure it could be reproduced with just a custom limited concurrency taskscheduler and little more knowledge than I have off the top of my head.

Here I'm showing CreateConnection working on the main thread, but then getting blocked when run in the background by the ConnectionTestActor.

https://github.com/michac/RabbitSchedulerTest

using Akka.Actor;
using Akka.Hosting;
using Microsoft.Extensions.Hosting;
using RabbitMQ.Client;

Console.WriteLine("Hello, World!");

RabbitHelper.TestRabbitConnection();

var builder = new HostApplicationBuilder();

builder.Services.AddAkka("sample-system",
    builder => { builder.WithActors((system, _) => { system.ActorOf<ConnectionTestActor>(); }); });

var host = builder.Build();

await host.RunAsync();

class TestMessage
{
}

class ConnectionTestActor : ReceiveActor
{
    public ConnectionTestActor()
    {
        ReceiveAsync<TestMessage>(OnTestMessageAsync);
    }

    protected override void PreStart()
    {
        Self.Tell(new TestMessage());
    }

    private Task OnTestMessageAsync(TestMessage arg)
    {
        RabbitHelper.TestRabbitConnection();

        return Task.CompletedTask;

        // Running the CreateConnection method with Task.Run
        //   sets the current scheduler back to the default
        //   so the MainLoop can start.
        // await RabbitHelper.TestRabbitConnectionAsync();
    }
}

public static class RabbitHelper
{
    public static async Task TestRabbitConnectionAsync()
    {
        try
        {
            Console.WriteLine("Invoking: TestRabbitConnection");

            var factory = new ConnectionFactory()
            {
                Password = "password",
                UserName = "tester",
                Port = 5672,
                HostName = "localhost"
            };

            var connection = await Task.Run(() => factory.CreateConnection());

            Console.WriteLine("Connected!");

            connection.Close();
        }
        catch (Exception e)
        {
            Console.WriteLine(e);
        }
    }

    public static void TestRabbitConnection()
    {
        try
        {
            Console.WriteLine("Invoking: TestRabbitConnection");

            var factory = new ConnectionFactory()
            {
                Password = "password",
                UserName = "tester",
                Port = 5672,
                HostName = "localhost"
            };
            var connection = factory.CreateConnection();

            Console.WriteLine("Connected!");

            connection.Close();
        }
        catch (Exception e)
        {
            Console.WriteLine(e);
        }
    }
}

Expected behavior

It's not clear that CreateConnection is going to inherit any async context from the thread it's run on. I would expect the MainLoop to either have a dedicated thread or at least make sure it runs on the ThreadPool, which can be done by specifying the Default TaskScheduler in Task.Factory.StartNew.

I am remediating this for now by wrapping my calls to CreateConnection in a Task.Run to make sure the TaskScheduler is the ThreadPool.

Additional context

Here is an article on the risks with using StartNew for reference: https://blog.stephencleary.com/2013/08/startnew-is-dangerous.html

lukebakken commented 1 year ago

Great, thank you for all of the information! I suspect we can address this in the next minor release.

lukebakken commented 10 months ago

@michac - please see this PR - https://github.com/rabbitmq/rabbitmq-dotnet-client/pull/1388

Is that what you're thinking?

michac commented 10 months ago

Yes, this is great, thank you!

lukebakken commented 10 months ago

@michac - thanks, I will test the PR using the project you provided.

lukebakken commented 9 months ago

@michac sure enough, my PR allows both connections to succeed in your test app.

lukebakken commented 9 months ago

Fixed by #1388