Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Raise a warning on multiple rapid connections / disconnections #224

Open
RobertCraigie opened this issue Jan 15, 2022 · 6 comments
Open
Labels
kind/improvement An improvement to existing feature and code. level/intermediate priority/medium topic: client Related to the Client API
Projects

Comments

@RobertCraigie
Copy link
Owner

Problem

Currently an easy mistake to make is to use the client's context manager feature on every database query e.g.

async with client:
    user = await User.prisma().find_unique(
        where={
            'id' user_id,
        },
    )

This is a mistake when used in anything other than a small seed script as it will create a connection pool and connect to the database only to close the pool and disconnect immediately afterwards. This is very inefficient and should only be done once per application runtime.

Suggested solution

I believe this mistake is easy to make as the initial connect() must be called explicitly by the user and sometimes it is not very obvious where to do this. (#103 will solve this).

We should update the documentation for connecting with the context manager with a big warning saying not to use this repeatedly in actual applications.

We should also consider raising a warning if many calls to connect() and disconnect() are encountered. There should also be an easy way to disable these checks, maybe a new client parameter?

Additional context

Encountered this pattern while reading through open source code:

https://github.com/shydiscord/shy-dashboard/blob/f708c3a70d9e3aaa60f6bae693bf649d27883ea0/shy-dashboard/server/blueprints/api.py

@RobertCraigie RobertCraigie added the kind/improvement An improvement to existing feature and code. label Jan 15, 2022
@RobertCraigie RobertCraigie added this to To do in v1.0.0 via automation Jan 15, 2022
@tday
Copy link

tday commented Feb 9, 2022

@RobertCraigie the flask example app scopes the client to each request which is required given that Flask's async views scopes the event loop to each request (from my understanding). The example also doesn't actually await the async connect so there are possible race conditions.

If my understanding is correct, then this means that we'd need to create a new connection pool for every request which seems problematic for performance (time to create the connection, and ultimately a large number of connections to the DB as requests scale up). Is that understanding correct?

If so, any thoughts on how to create a global connection pool with the current design of the this library?

@RobertCraigie
Copy link
Owner Author

RobertCraigie commented Feb 9, 2022

@tday The flask example app uses the synchronous version of the client so you don't actually need to await the the connect but looking back through it, you are right, there is a race condition on the connect call. It needs a threading lock.

If my understanding is correct, then this means that we'd need to create a new connection pool for every request which seems problematic for performance (time to create the connection, and ultimately a large number of connections to the DB as requests scale up). Is that understanding correct?

You are correct, that would be very problematic for performance.

If so, any thoughts on how to create a global connection pool with the current design of the this library?

Does doing something like this before running the app work for you? I haven't properly used async flask before.

import asyncio

prisma = Client(auto_register=True)
asyncio.get_event_loop().run_until_complete(prisma.connect())

I cannot remember why I chose to not connect to the database until a request came in when originally writing the flask example, it should probably be rewritten.

@tday
Copy link

tday commented Feb 9, 2022

The flask example app uses the synchronous version of the client so you don't actually need to await the the connect but looking back through it, you are right, there is a race condition on the connect call. It needs a threading lock.

Ah, I see. I missed that there is a config to make the client synchronous in schema.prisma. The example implementation will work as is then.

Does doing something like this before running the app work for you? I haven't properly used async flask before.

This will get the client to connect, but the client will still execute outside the event loop context of requests so you'll see errors like "Event loop is closed". I tried a similar approach using Flask's ensure_sync


I'll follow up with how I solve this to provide some clearer feedback on how the library might support it, if at all.

@RobertCraigie
Copy link
Owner Author

Ah, I see. I missed that there is a config to make the client synchronous in schema.prisma

Yes that is not very clear, I do plan on changing that in the future by supporting both synchronous and asynchronous clients at the same time.

This will get the client to connect, but the client will still execute outside the event loop context of requests so you'll see errors like "Event loop is closed

Hmmm that is unfortunate, I'll have a look too and see if I can do anything.

I'll follow up with how I solve this to provide some clearer feedback on how the library might support it, if at all.

That would be much appreciated thank you :)

@RobertCraigie
Copy link
Owner Author

@tday I've isolated this issue down to using HTTP connection pooling.

You can monkeypatch the library to remove the HTTP connection pooling although I do not know much this would effect performance (this may just have to be a limitation of using async flask).

import httpx
from typing import Any
from prisma._async_http import HTTP, Response

async def patched_request(
    self: HTTP, method: str, url: str, **kwargs: Any
) -> 'Response':
    async with httpx.AsyncClient(**self.session_kwargs) as session:
        return Response(await session.request(method, url, **kwargs))

HTTP.request = patched_request

You may also want to consider switching to Quart if this is a deal breaker for you.


I'll also create an issue to add an option to disable HTTP connection pooling without having to monkeypatch.

@tday
Copy link

tday commented Feb 9, 2022

Wow, thanks for digging in!

You may also want to consider switching to Quart if this is a deal breaker for you.

My plan is to try to switch to an ASGI supported server. Quart seems like a good option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/improvement An improvement to existing feature and code. level/intermediate priority/medium topic: client Related to the Client API
Projects
Status: Backlog
v1.0.0
To do
Development

No branches or pull requests

2 participants