Repository navigation
Exclusive connection openers get starved in multiplexing over-capacity mode #4180
Description
Activity
I took another look at this: shouldn't this lines guard specifically against running a multiplexing query on an idle connector?
npgsql/src/Npgsql/MultiplexingConnectorPool.cs
Lines 196 to 207 in 427940a
// We may be in a race condition with the connector read loop, which may be currently returning // the connector to the Idle channel (because it has completed all commands). // Increment the in-flight count to make sure the connector isn't returned as idle. var newInFlight = Interlocked.Increment(ref connector.CommandsInFlightCount); if (newInFlight == 1) { // The connector's in-flight was 0, so it was idle - abort over-capacity read // and retry the normal flow. Interlocked.Decrement(ref connector.CommandsInFlightCount); spinwait.SpinOnce(); continue; } (Also, we probably shouldn't grab a connector with 0 in-flight queries, like you've done in #4179)
You're probably right (though the impact of all this code is really hard to follow across the codebase so I'm not 100% sure).
Either way it'll be really inefficient if the write loop encounters a lot of binding scope connectors. They're not idle (so cannot be rented by the earlier TryGetIdleConnector) but they're not available for multiplexing either as they'll always fail this check.
As it stands we could even make the multiplexing loop spinloop forever by holding all connectors in a binding scope.
@NinoFloris @vonzshik I remember thinking about this stuff and also doing some stress-testing over over-capacity mode, though it's true we generally don't exercise it much (we obviously never do over-capacity in the TechEmpower benchmarks). I think this code should be OK, but it's worth taking another look at some point.
Either way it'll be really inefficient if the write loop encounters a lot of binding scope connectors. They're not idle (so cannot be rented by the earlier TryGetIdleConnector) but they're not available for multiplexing either as they'll always fail this check.
This is true... I remember considering another data structure only for unbound connectors, but it wasn't trivial.
Am putting this in 7.0 to think about.
I was thinking about making a separate pool for the case whenever a connection tries to bind a connector.
Interesting idea - I like it. I don't remember any more what seemed complex about separating bound and unbound connectors, but it's definitely worth revisiting.
General direction by @NinoFloris... In multiplexing, when someone gets to RentAsync, that means that there was no idle connection. At that point, we can pick some connection and flag it as reserved for this usage. This makes us refrain from pushing more over-capacity commands into it, which will eventually make it available. When that happens, the connection gets pushed into the idle connector channel.
- changed the title
[-]Guard against concurrent use in multiplexing over capacity mode and connection binding scopes[/-][+]Exclusive connection openers get starved in multiplexing over-capacity mode[/+]on Sep 25, 2022 Multiplexing was removed in #6457
Today over capacity mode is just looping over all connectors - idle or busy - without duly guarding against them being in use by, or concurrently starting, some binding scope like a transaction.