Added a synchronous write loop for connections.

[lukebakken]

Fixes #1354

Supersedes #1389

[stebet]

Looks good :)

[danielmarbach]

Is this worthwhile pulling in given the direction the client is heading to? To me that looks more like a bandaid that ignores the underlying sync blocking IO. Any configuration flag like this has to be explained to the users to make sure it is used in the right scenarios and then also be maintained and/or deprecated in the right ways.

To me by reading through the conversation thread in the linked issue I couldn't really find derive that this writer loop bandaid will actually solve much in the wild. If the root cause should be fixed wouldn't it then be better to try to backport some of the asyncification from v7 that can be brought in as pure internal potentially to free up the code path from synchronous IO?

[michaelklishin]

I don't have objections to adopting this for 6.x and moving full steam ahead with much more fundamental changes for 7.x.

The maintainers of this client understand that 6.x has certain fundamental limitations that can be considered unfixable.

There is also a parallel to draw with the Khepri migration in RabbitMQ itself. RabbitMQ 3.x has very old design decisions that only a 4.x can really address. Even though in the end Khepri will be an option in 3.13+, which can be considered a very large "band-aid", all future plans assume a reasonable number of breaking changes exactly because a patchwork of band-aids can only take you so far.

So I vote for merging this and focussing on 7.x. @danielmarbach does this make sense?

[WeihanLi]

Since the default value is false, maybe we could remove the init value

Suggested change

	public bool EnableSynchronousWriteLoop { get; set; } = false;
	public bool EnableSynchronousWriteLoop { get; set; }

[michaelklishin]

Making the default more visible is a good idea. I'd not remove it.

[WeihanLi]

Overall, I think it's a kind of code style. I prefer to remove it while I would not argue for this.

Have a test on this, the default value would be removed when built in release mode, but would not when in debug mode

[danielmarbach]

This is still gold ;)

https://github.com/davidfowl/AspNetCoreDiagnosticScenarios/blob/master/AsyncGuidance.md#avoid-using-taskrun-for-long-running-work-that-blocks-the-thread

[WeihanLi]

When just using the Task.Run, the thread would be borrowed from the thread pool, the long-running task on a thread-pool thread would affect the thread-pool thread scheduling.
Here's the LongRunning flag is specified, it would use a separate thread, so I think it's ok.

https://github.com/dotnet/runtime/blob/93381bf9c745e5925fb68ac4da2b38526b36222a/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/ThreadPoolTaskScheduler.cs#L42