Fix for AsyncDatabase and parallel transactions #888 ?

[DenesPal]

async_.core.AsyncDatabase was incorrectly subclassed from threading.local, that did not make it's properties context-local, leaking state (like session and transaction states) across async contexts.

I am assuming that sync_.core.Database implementation is correct.
Apart from the two class-level global mappings (_NODE_CLASS_REGISTRY and _DB_SPECIFIC_CLASS_REGISTRY), all instance properties of Database are threading-local.
So I've converted the same instance properties of AsyncDatabase to ContextVar @property-s (because ContextVars work with .get & .set).

[DenesPal]

I'm wondering if all properties of AsyncDatabase should be context-aware?
pid would be the same for all contexts as the process does not change. (Same applies for thread-local Database.pid)
I'm wondering if it is practical changing the database driver/URL in an async context, expecting that separate contexts carry on with different connection. If not then url, driver, _database_name, _database_version, _database_edition could also be normal instance properties.

[robsdedude]

Disclaimer: I'm not a maintainer of neomodel, but I've kindly been asked for input. So here goes:

I'm wondering if all properties of AsyncDatabase should be context-aware?

From a general maintenance perspective I suggest to keep the async and sync implementations as close as possible from a mental perspective. So at least for this PR, I suggest to turn everything that's thread local in sync into context local in async. The PR could (optional) even go a step further and also use ContextVars for the sync implementation because according to the Python API docs each thread gets its own context. So that should be equivalent. Actually, if I remember correctly, the sync code is auto-generated/transpiled from the async code (see /bin/make-unasync). Feels like the CI should've complained about this, but I guess there's no job setup for such check 🙃

Another thing to keep in mind is that async Python has some gotchas/rough edges. What do you think this snippet does?

import asyncio
from contextvars import ContextVar


var = ContextVar('var', default='default')


def print_var():
    print(f'var: {var.get()}')


async def task():
    print("task:")
    var.set('task')
    print_var()


async def main():
    print("main:")
    print_var()
    await asyncio.wait_for(task(), timeout=1)
    print("main:")
    print_var()


if __name__ == '__main__':
    asyncio.run(main())

Does tasks var.set affect what main sees?

This is a trick question... Until Python 3.11 it doesn't, because asyncio.wait_for in those older versions spawns an internal task, while in newer Python implementations it doesn't. In threading terms this would be equivalent to some innocent looking standard lib calls spawning a thread under the hood. I frankly don't know if there are other asyncio functions that do this. That's detrimental. Imagine some code like this:

async def save_node():
        node = TestNode(name="Cool node")
        await node.save()

async def main():
    async with adb.transaction:
        # don't want to wait for ever to create the node
        asyncio.wait_for(save_node, timeout=1)

That reads alright and does what it looks like on the tin for newer Python version, but on older Python verions save_node will not be running inside main's transaction.

I don't think this is a blocker for this PR, but it needs consideration and probably some warning in the docs.

[mariusconjeaud]

Actually, if I remember correctly, the sync code is auto-generated/transpiled from the async code (see /bin/make-unasync)

Indeed it is auto-transpiled, with ways to have custom code for either async or sync ; both of which mechanisms I took from the Neo4j python driver you know so well !

Based on your comments (thanks a ton), then it sounds like moving everything in AsyncDatabase to ContextVar and letting it get transpiled works. I will update the PR with the transpiling, then test it, and set it for merge on the next "major" release - in case there are breaking changes we did not catch. Is that OK @DenesPal ?

Feels like the CI should've complained about this, but I guess there's no job setup for such check 🙃

Well, what can I say ? Checking that the transpile happened is a good idea ! I have been relying too much on my pre-commit actions here.

[mariusconjeaud]

So... Looking good for me now ; the new tests pass ; no existing tests were broken, except those 3 that look totally unrelated, and work perfectly fine in my local environment ! Gah !

(Note I finally managed to fix the Aura tests though)

[mariusconjeaud]

OK tests are fixed, it was an issue with a previous PR that had duplicated code which I had missed

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud