[WIP] New pooling for mysql and asynpg

[abondar]

Closes #140

[grigi]

Earlier I used a lock _lock in the db clients to enable concurrency. The tortoise.backends.base.client.ConnectionWrapper context manager manages the locks for now.
Consider refactoring it to allow you to aquire the lock in the get_or_create?

The lock mechanism is a bit heavy handed right now, so I'd welcome a more elegant solution.

[abondar]

@grigi Could you help me out there please?
I couldn't resolve issue with mysql reconnect test, because I didn't quite understand how it should work :)
Also there are some compatibility issues for python3.5 which I can't find time to resolve

If you currently have time to have a look at it - it would be great, cause I think this PR bring quite important functionality

[grigi]

I'll have a look.

I agree this is important. I tried a few times to "fix" this issue, but it was causing trouble.

One solution would be to drop Py3.5, as it is going to be EOL'ed in a few months?
Also, then we can start using some of the nicer py3.6 syntax.

[grigi]

If this is broken, then practically everything to do with transactions are broken…

[grigi]

Oh wow, it creates a new connection, and does not honour the current connection.
This is with py3.7 and postgresql on my system.

[grigi]

The context is not kept. Basically the same issue I had with my attempts.

[grigi]

I think fundamentally we don't manage the context correctly. Or rather, the way we try and map a sync-like style of functionality to async world is a problem.
The contextvars assumes that your changed context is only valid in your current scope or children. The way we do transactions, it is via a function call to something that changes the context in the db-client, then when it returns, we lost the context already.

A possible situation is to allow transparent transactions only for:
async with in_transaction(): or @atomic. Because there we can control the stack.
For await start_transaction() we always lose the context.

This is part of the reason I wanted to change the whole transactionwrapper pattern, as it becomes super difficult to reason about it.

In a threaded context the contextvar is thread-local. For coroutines it is scope-local, and unless we carefully control the entiere scope it is going to be impossible.

Also:

This PR changes so much, I have difficulty seeing what it is you even did :-(
I'd really try and keep the changes re concurrency model clean, because it is so troublesome.

[abondar]

The contextvars assumes that your changed context is only valid in your current scope or children. The way we do transactions, it is via a function call to something that changes the context in the db-client, then when it returns, we lost the context already.

🤦‍♂ Feel really stupid because I never realised it before

A possible situation is to allow transparent transactions only for:

I think we could leave (or somehow rename) start_transaction, it will return transaction object, so it could be used with manual using_db syntax. That way it still will be possible to manually work with transactions in some harder cases, that contextmanager or decorator can't handle

This PR changes so much, I have difficulty seeing what it is you even did :-(

Real changes are moving ContextVar into db client instances, adding pools instead of connection for asyncpg and mysql, fixing get_or_create, so it will feel "atomic"

Also I changed default test runner to pytest, mostly because more I was working with green more dumbfounded I became, so I decided to try using pytest as default, because as I see - pytest is default test runner for many python users and it will be more familiar for potential contributors as well

[grigi]

🤦‍♂ Feel really stupid because I never realised it before

This took me a LONG time to realise. Only after I did the concurrency fixes without any pooling did it become apparent.

I think we could leave (or somehow rename) start_transaction, it will return transaction object, so it could be used with manual using_db syntax. That way it still will be possible to manually work with transactions in some harder cases, that contextmanager or decorator can't handle

Yes, we will likely have to break API here. That's OK. If we break compat here, we couls add a deprecated-exception so that people know where to look for the change.

Real changes are moving ContextVar into db client instances, adding pools instead of connection for asyncpg and mysql, fixing get_or_create, so it will feel "atomic"

Yes, I have similar thoughts. A single DB client should manage its own state. The issue is how do we pass an equivalent of uisng_db in? It used to be a class object, now it may have to be whatever is wrapped by ContextVars?

I would strongly recommend to try fix the transaction consistency issues before adding connection pooling, because once we can manage context, the pool stuff becomes easy.
Or at least with a pool size of 1.

Note the expected-failure test case in tortoise/tests/test_concurrency.py (test_concurrency_transactions_create) → That one breaks because transactions are not handled with enough isolation (ContextVar issue), and will even fail with a single connection on SQLite.

So I'd test the transaction changes to get that to work right first, just to minimise the side-effects and keep the changes focused.

Also, note that py3.7 has much more fine-grained context-vars behaviour so if you can get it working on that, and then with 3.6 with the polyfill, I'll be ecstatic.

Also, I saw you generated the requirements on my 3.5. (black was excluded). I used to always run this on py3.6, but after changing my dev environment to 3.7 I had to do what is in this commit: b4a9ce7
(That branch also does a lot of the linting related fixes, I'm still trying to replicate the two reported issues on 0.12.7, somehow it is easier to get side-tracked than focus on the issue if you don't have a narrow set of parameters)

[15390413627]

@grigi hi, I am using tortoise in my company's project now, it requires very high concurrency, thus connection pool is required. I've tried to replace backend/base/client.py and backend/asyncpg/client.py with this PR, did a little pressure test, and it worked out pretty fine. So basically I was wondering why are you so concerned about the contextvars inside base/client.py, cuz from what I see, it first initialized as the AsyncpgDBClient object, then when a transaction started, the _current_transaction is set to the current TransactionWrapper object, then when trying to finish the transaction, the _current_transaction is set to AsyncpgDBClient back again, could you please tell me what is the use case of this contextvars and what potential problems could it cause,. btw, I am using postgresql for my project. Appreciated!

[grigi]

Hi @15390413627 There is some more background to what was going on here: #69 (comment)

But generally the problem comes down to we need to provide support. So it needs to work well for every supported situation. So, I don't want to put something out there that I know is broken badly for one of our supported cases.

One of the issues with the current system is concurrent accessing when transaction is going on, some context leaks across the transaction boundaries, which is unacceptable for some systems, and acceptable behaviour for others.

I'll have a look again. We gave decided to deprecate py3.5, which opens up more modern ways of doing async context managers, which I think should help resolve the last pressing issue, as we should now have more control over the stack.

As an intermediate solution, how do you propose we do this to support your use case over the next while until we get everything perfect?

[Quinn-Fang]

Hi @grigi, I am @15390413627, so don't get confused, I just don't like that phonenumber name github gave me ... anyways, In order to make my project work, and 2 days struggle, I finally make it work. So check it out #176, I did this only to satisfy my need, high concurrency support for postgresql only. I only did pressure test, I did not run the unit tests cuz the weird problems on my macbook. So this is my intermediate solution until you guys make everything alright. So what do you think?

[grigi]

Ah, @Quinn-Fang Thanks for clearing that up. I wondered about the weird number. I'll look at it :-)
Scratching your own itch is a big part of open source 😄

[grigi]

superseded by #229