New pool (WIP) #69
New pool (WIP) #69
Conversation
Pull Request Test Coverage Report for Build 349
💛 - Coveralls |
1 similar comment
Pull Request Test Coverage Report for Build 349
💛 - Coveralls |
|
There is several useful changes in this PR that I'm going to pull out in its own PR:
|
|
@abondar Also whilst debugging the idiotic autocommit issue, I found that we don't have a clear expectation of how transactions should operate. I'm proposing this:
|
This takes the useful parts from #69: * contextvars update & testrunner fixes * db_url improvements * minor test enhancements * dependency updates * autocommit for MySQL * re-ordered execute-SQL log to be inside connection context manager for less confusing logs.
This takes the useful parts from #69: * contextvars update & testrunner fixes * db_url improvements * minor test enhancements * dependency updates * autocommit for MySQL * re-ordered execute-SQL log to be inside connection context manager for less confusing logs.
This takes the useful parts from #69: * contextvars update & testrunner fixes * db_url improvements * minor test enhancements * dependency updates * autocommit for MySQL * re-ordered execute-SQL log to be inside connection context manager for less confusing logs.
|
@abondar I rebased and simplified this connection pooling PR. |
|
@grigi For some reason python3.7/mysql build failed crazily, but other builds are now fine. I'll look into failed build later if you won't manage to do it before me |
|
My first thought was "Probably contextvars" as the backport |
|
It doesn't use the transactioned connection, but a different one: It creates a transaction on connection This should be unrelated to your queryset changes. |
|
Yup, it was already broken on my rebase, and was working before then. |
|
Ok, I did some more digging, and found that I was mistaken. py37+mysql (pooling) never isolated right. There is some documentation re the differences here: https://pypi.org/project/aiocontextvars/ |
|
Currently the client looks up a ContextVar that exist globally, outside of its own scope. I think the common issues for globals may actually be relevant here. So I'm going to consider removing the global state tracking and try and simplify the transaction handling in the Client. To make it easier to reason about things. It is easy to trigger the current issue, and it appears to be a very easy race-condition. Unfortunately the code isn't that easy to reason about, hence my attempt at simplifying it some more. |
I don't really understand why there is problem with contextvars, shouldn't it be okay for them to be global, because that what it shows in examples in docs https://docs.python.org/3/library/contextvars.html ? |
|
Yes, the docs talk of globals, but how I see it being used in Gino is as part of a instance-variable. Also, the way transactions sort-of replace the class just doesn't feel clean, hence me considering refactoring it. |
|
Hi, guys. I am using tortoise ORM (PostgreSQL backend) in project. Now I am considering applying database pool (asyncpg pool) in tortoise. It seems that in 'new_pool' branch, you guys have already implemented pool for MySQL.
But these fails seem irrelevant to database pool. I would like to know the progress of this feature. Are problems you talked in this issue still unsolved? If I add pool for PostgreSQL, is there anything tortoise-specific I need to know about? Can I start with this branch? Thanks for your excellent work for this library! |
|
Hi @zhoufeng1989 So, for a short history as to why this have been sitting around so long: We then had a few concurrency related bug reports, and I managed to fix all of them quite easily except for one. That was when mutex around a transaction. The fail was identical to this. The obvious difference is that async contextvars is built-in on Py3.7, and a monkey patch on 3.5/3.6 So it turns out that the context gets applied at higher resolution on py3.7, so would be tied to stack level, instead of to co-routine scheduling. So we need to control the stack a bit better so that we can enable this reliably. I suppose that the "working way" on 3.5/3.6 was probably not entirely reliable. Then life interfered (Kid got very sick) but that is gradually returning to normal. So for a while most of the dev work was done by contributors, and only recently (the last 2 months or so) I actually did real work on this again. If you read on #141 Andrey re-attempted it, but ran ultimately into the same issue, and after discussing the core issue as I see it (and I may be wrong) he agreed that we may have to do a large refactor to fix the stacking issue. So, yes, if you want to contribute in any way to help us reach the milestone reliably, I would be super grateful 😀 I would start with that commit I linked, and see how restructuring the code, could enable the stack-level resolution of the built in contextvars to work. Wow, that was a lot longer than I intended, but I hope it gives enough context. |
|
Hi @zhoufeng1989 So, for a short history as to why this have been sitting around so long: We then had a few concurrency related bug reports, and I managed to fix all of them quite easily except for one. That was when mutex around a transaction. The fail was identical to this. The obvious difference is that async contextvars is built-in on Py3.7, and a monkey patch on 3.5/3.6 So it turns out that the context gets applied at higher resolution on py3.7, so would be tied to stack level, instead of to co-routine scheduling. So we need to control the stack a bit better so that we can enable this reliably. I suppose that the "working way" on 3.5/3.6 was probably not entirely reliable. Then life interfered (Kid got very sick) but that is gradually returning to normal. So for a while most of the dev work was done by contributors, and only recently (the last 2 months or so) I actually did real work on this again. If you read on #141 Andrey re-attempted it, but ran ultimately into the same issue, and after discussing the core issue as I see it (and I may be wrong) he agreed that we may have to do a large refactor to fix the stacking issue. So, yes, if you want to contribute in any way to help us reach the milestone reliably, I would be super grateful 😀 I would start with that commit I linked, and see how restructuring the code, could enable the stack-level resolution of the built in contextvars to work. Wow, that was a lot longer than I intended, but I hope it gives enough context. I want that issue resolved, because once we just get the locking around transactions done, we now have prefect isolation. |
|
Hi @grigi , thanks for your informative reply, but I still feel kind of confused about the problem that fails on Py3.7 but works on Py3.5 & Py3.6. I am trying to reproduce the problem, but got some other problems. all my work is based on the latest release 0.13.0.
Thanks! |
|
Yes, some tests manually create DB contexts, e.g. for testing db-specific DDL generation. (I think it should not actually create a DB instance, just a client object). It should be limited to I can't remember exactly which ones were failing on py3.7, but the most important tests for pooling/concurrency is |
Now that code base is simpler, and test runner should be more sane, attempt to add connection pooling for the third time.
The plan is to change to connection pooling ONLY, as we currently implement persistent connections, but only one persistent connection.
A connection pool should:
Things done:
Concerns: