Made multiplexing bootstrap threadsafe - #3257

@vonzshik this looks good, but can you give a short explanation on how the lack of thread-safety here generates an error? Is this because of the idle channel perf issue you mention in #3248 (comment)? I'm still not sure exactly how it's all connected.

I've done more testing. The problems seems to be coming from the transaction logic. Or to be more clear, how multiplexing works, when we attemp to get a connection from the pool synchronously.
I'm talking about this:

I was busy for the last few days, and will take a look at everything else later today (or tomorrow).

That is, if there was one

Oh great point... Before multiplexing, beginning a transaction is indeed always synchronous - we just write some bytes into our write buffer, but don't actually send. But with multiplexing, starting a transaction means opening a connection 🤯

We should definitely fix this, it should be easy.

I still don't get why this is necessary to prevent any errors though...

Honestly, I'm not sure myself. Also, I've found another 'fun' thing:

Okaaay, I've managed to repeat the same issue without npgsql. @roji do you want to take a look?

Okaaay, I've managed to repeat the same issue without npgsql. @roji do you want to take a look?

Absolutely... are you saying there's an issue with Channels? If you share the repro I can take a look, and if needed you can post it in runtime too...

Okaaay, I've managed to repeat the same issue without npgsql. @roji do you want to take a look?

Absolutely... are you saying there's an issue with Channels? If you share the repro I can take a look, and if needed you can post it in runtime too...

Here you go. For now, I would like for you to take a look, as it's possible I've missed something. But it should be more or less equal to how npgsql works.

The main thing seems to be:

1. Attempt to read asynchronously from the channel
2. Do some async work (so we move to the thread pool)
3. Return object to the channel
4. Attempt to read synchronously from the channel (AsTask().GetAwaiter().GetResult()).

After that, we essentially work in a single thread mode, and the channel returns something every 1 second.
Also, if the first part is synchronous, the second part is removed (or moved after the third part), or the fourth part is asynchronous, then everything is fine.

@vonzshik I'm investigating your repro and I think I understand what's going on. The problem is (not surprisingly) not with Channels, but more with us insisting on trying to build a sync API (NpgsqlConnection.Open) over an async one (Channels). I'm looking for a solution.

@vonzshik I'm investigating your repro and I think I understand what's going on. The problem is (not surprisingly) not with Channels, but more with us insisting on trying to build a sync API (NpgsqlConnection.Open) over an async one (Channels). I'm looking for a solution.

So it's more of a "channels were not designed to work like this, so don't do that" kind of thing?

So it's more of a "channels were not designed to work like this, so don't do that" kind of thing?

Not quite - or maybe only in a limited sense :) Doing sync-over-async in general (regardless of Channels) is problematic and risky, and I wish we didn't have to do it. However, our (new) pooling implementation is based on Channels - which is async only - and yet we do not want to break the entire world by saying that Npgsql doesn't support sync any more. I was worried about this when I first adopted Channels (as a way of significantly reducing pool lock-free complexity). The good news is that I think I found a reasonable fix; I've done a bit of tweaking on your repro, take a look at https://github.com/roji/Test/tree/VonzshikChannelFun.

I'll assume you understand SingleThreadSynchronizationContext - it's about making sure that we can execute an async operation without its continuation running on the TP thread (but running instead on some other thread). This allows us to block synchronously on the operation, because if that happens on a TP thread, it doesn't trigger a deadlock where all TP threads are synchronously blocking on the operation, but its continuation is waiting to be executed on the TP.

Issue 1

The problem is that ValueTask.AsTask schedules a callback on the IValueTaskSource - the one which completes the Task - without regards to the synchronization context. In effect, us using AsTask introduces exactly the dependency on the TP which we need to avoid. I don't think this can be considered an issue in AsTask itself: the synchronization context is supposed to affect continuations introduced by await, and it will indeed affect continuations on the Task returned by AsTask, but will not affect the continuation on ValueTask which transitions the Task.

My fix for this is to replace reader.ReadAsync().AsTask().GetAwaiter().GetResult() with the following (see here):

var mre = new ManualResetEventSlim();
reader.WaitToReadAsync().GetAwaiter().OnCompleted(() => mre.Set());
mre.Wait();

Here we set up the callback on the IValueTaskSource ourselves (similar to what AsTask does under the hood), but GetAwaiter().OnCompleted() does respect the synchronization context. I've submitted #3268 to fix this.

Issue 2

In https://github.com/roji/Test/tree/VonzshikChannelFun, the first async rent/return pair is commented out, and only the second sync pair is executed. If you uncomment the first pair, you will still see a starvation regardless of the AsTask issue above. This is "normal", and is a result of mixing sync and async (never a good idea), coupled with a thundering herd effect.

• Since async rent is the first one in the function, the program immediately calls await RentAsync a lot. Since the pool is starved (almost no objects), this causes tons of continuations to be enqueued for that in the TP global queue.
• Each TP thread that comes out of one of these continuations returns an object to the pool, but then blocks synchronously (the 2nd, sync rent).
• The object that was returned to the pool unblocks one of the async continuations, since those are enqueued first on the channel (which works in FIFO). That thread goes through the previous point, returning one object and then blocking synchronously.
• Because there are a lot of enqueued continuations for the first, async rent (thundering herd effect), you have to wait a long, long time before the first sync rent attempt gets unblocked. Until that point all your TP threads are synchronously blocked, and the TP is hill-climbing slowly (adding about one TP thread per second).

To summarize, this is an issue in user code, rather than something that we can fix in Npgsql.

/cc @Brar @YohDeadfall

Wow.

Thank you @roji for an explanation! Now this does make a lot of sense.

@vonzshik so can you confirm that this PR isn't necessary to fix any issues (after #3268), right (can you test)? Am still totally fine with merging it, just want to make sure we understand everything.

@vonzshik so can you confirm that this PR isn't necessary to fix any issues (after #3268), right (can you test)? Am still totally fine with merging it, just want to make sure we understand everything.

Not entirely unnecessary. As you've said, the second issue is mixing of sync and async code. And this pr makes the BeginTransactionAsync truly asynchronous.

And this pr makes the BeginTransactionAsync truly asynchronous.

Ah yes, that part of it - for sure. But not multiplexing bootstrap thread safety, right?

And this pr makes the BeginTransactionAsync truly asynchronous.

Ah yes, that part of it - for sure. But not multiplexing bootstrap thread safety, right?

Yep - it's mostly an optimization.

Have approved, you can merge when ready. Note that we usually squash PRs here, and almost never do merge commits (why complicated the git history...).