IIRC the intention here was to provide better guarding against (incorrect) concurrent use of (non-multiplexed) connections by the user, by calling EndUserAction as late as possible (this is what releases the user lock). But since Cleanup releases multiplexed connections back to the pool, it's obvious that we should call EndUserAction after that.

We should rethink the general concurrency detection strategy - keeping both multiplexing and non-multiplexing in mind. I've opened #3255 to track this, but I think that's definitely not for 5.0. For now I'm good with this change.

(a Rust-like language ownership model would have been very useful here :/)

We should look into doing unbind as part of EndUserAction. I've had thoughts of linking the two mechanisms, but you're showing how important that is.

I might be misunderstanding what you're trying to do, but I'm not sure this mechanism is going to work. Each instance of NpgsqlTransaction belongs to its connector, and is recycled (Init is called when a new transaction starts). This means that if the connector is returned to the pool and then immediately given out again, and a new transaction is started, isCompleted will be set to false.

The only proper way to fix these race conditions is never do anything to the connector after we return it to the pool. I think this means redoing EndUserAction and scope unbinding to link them together, as mentioned above.

If I'm correct and you agree with this direction, I propose we leave this PR open for now and not do these changes (since the above will affect everything anyway). Except for the connector.Connection = null fix above, which is unrelated.

I might be misunderstanding what you're trying to do, but I'm not sure this mechanism is going to work. Each instance of NpgsqlTransaction belongs to its connector, and is recycled (Init is called when a new transaction starts). This means that if the connector is returned to the pool and then immediately given out again, and a new transaction is started, isCompleted will be set to false.

This is something, I've come to realize today, while I was doing more testing 😢.

The only proper way to fix these race conditions is never do anything to the connector after we return it to the pool. I think this means redoing EndUserAction and scope unbinding to link them together, as mentioned above.

Maybe. Or maybe we can go the way @YohDeadfall proposed for the NpgsqlDataReader.

If I'm correct and you agree with this direction, I propose we leave this PR open for now and not do these changes (since the above will affect everything anyway). Except for the connector.Connection = null fix above, which is unrelated.

You are correct and I do agree - multiplexing is hard (even though it's cool), and this fix can wait.

The only proper way to fix these race conditions is never do anything to the connector after we return it to the pool. I think this means redoing EndUserAction and scope unbinding to link them together, as mentioned above.

Maybe. Or maybe we can go the way @YohDeadfall proposed for the NpgsqlDataReader.

You mean we should consider to not recycle NpgsqlTransaction anymore? Sure, but that's the easy way out 🤣 One of the reasons Npgsql has such good perf (it's one of the reasons .NET Core is so high on TechEmpower) is because we try to pay attention to every little detail etc.

I've opened #3260 to track looking into this - unless you're very interested I can take this on.

Note: I do think we should do something here before releasing 5.0. At minimum we can avoid starting a user action altogether if in multiplexing.