-
Notifications
You must be signed in to change notification settings - Fork 67
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
Support multiple event loops with a given Connector
#1107
Comments
Hi @colonelpanic8 😄 Thanks for the detailed explanation of the issue you are currently seeing.
I'm trying to understand the use-case here as I've never run into this error myself. Do you mind sharing the piece of your application code where you are creating the The above info would be beneficial to wrap my head around whether this a config issue or indeed a bug. Thanks in advance 😄 |
The way our stuff is configured is kind of complicated, so I can't really just copy paste our stuff in a way that's going to be easy for you to understand. I'm super sure about my understanding of the underlying issue though. It will occur whenever multiple event loops are used with a single connector. You could certainly make the argument that the Async Driver Usage section in the readme sort of suggests that you need to be on the event loop that you want to use when you initialize things:
the thing is that it goes on to say:
which doesn't really make it clear that a connector that does so can't ever be used in an async context. In the case of my application, we actually use multiple different event loops for different things. I think that if this use case is not supported it should be made clear that:
I also think it would be relatively easy to simply make this sort of thing impossible by simply requiring that connection instantiation always happen on the correct event loop. On the other hand, I do think it may actually be possible to make a single connector support multiple event loops, but perhaps that is not the best thing to do. It's not totally clear to me to what extent this is even supported by sqlalchemy's async facilities. |
Also, one detail that I should make clear here is the following: Everything works fine as currently configured the vast majority of the time. It is only when we encounter an intermittent error with connection creation that we encounter this issue. |
Thanks @colonelpanic8 again for the additional details.
I think those two statements are good summaries of the issue at hand. Today we do not support connections across multiple event loops for a single @colonelpanic8 in your use case are you initializing a I'm trying to think if we want the I guess we could maybe have This may be related to #969 or even potentially a duplicate, as essentially |
No. I'm initializing an async_engine factory with a link to the connector on a dependency injection object. Then different objects use this engine factory to produce engines, possibly on different event loops.
I don't think this is very messy at all. its basically just a couple of lines. This is basically the sort of thing that I have in mind. I still actually think there is another option, which is to simply use asyncio.run_coroutine_threadsafe to run the connect_async on the CORRECT event loop and then await that future. This would make it perfectly fine to use a single connector across multiple threads. That does open a whole can of worms though, and it would require careful thinking about the details of the implementation. |
As far as I can tell. This is the only real reason this is a requirement, and I think that sharing of keys like that could pretty easily be accomplished without sharing through the ClientSession object. |
Connector
Hi @colonelpanic8 I've changed this into a FR and not a bug as this is a feature we don't support currently (I'll put up a PR with small comment/docs fix making this more clear). For now I would recommend creating a
Are the different event loops running in different threads? I assume so but just wanted to make sure. Otherwise I would suggest just using a single event loop for your use-case. Thanks again for bringing this to our attention 😄 |
yes. I still think there is a real "bug" here, because its very easy to use things in a way that is going to cause issues when the connections later get refreshed. Doc fixes are great, but its super easy to do a simple check that just says hey are we on the right thread/loop and then give a more comprehensible error. |
@colonelpanic8 I put up a WIP PR to showcase the kind of error we could raise. Let me know if you think it looks like an actionable error message 😄 |
Bug Description
This can occur when sqlalchemy attempts to create a new connection in the context of event loop that is not the event loop that cloud-sql-python-connector is using.
The connector has a member
_client
:cloud-sql-python-connector/google/cloud/sql/connector/connector.py
Line 262 in de2852f
the code is careful to make sure to instantiate this client in the correct async context (in the running event loop). The reason this has to be done lazily is really just because the init of CloudSQLClient initializees an aiohttp.ClientSession:
cloud-sql-python-connector/google/cloud/sql/connector/connector.py
Line 262 in de2852f
ClientSession must be instantiated on the even loop it will be used on because of this:
https://github.com/aio-libs/aiohttp/blob/f662958b150a9d8d92fcbd0c9235e6bee1bedd67/aiohttp/client.py#L257
now, whenever a request is made with this client session, a TimeoutHandle is created, parameterized by self._loop:
https://github.com/aio-libs/aiohttp/blob/f662958b150a9d8d92fcbd0c9235e6bee1bedd67/aiohttp/client.py#L443
This propagates to a TimerContext here:
https://github.com/aio-libs/aiohttp/blob/f662958b150a9d8d92fcbd0c9235e6bee1bedd67/aiohttp/helpers.py#L651
which we enter here:
https://github.com/aio-libs/aiohttp/blob/f662958b150a9d8d92fcbd0c9235e6bee1bedd67/aiohttp/client.py#L474
now when we enter this timer context we try to find the current task:
https://github.com/aio-libs/aiohttp/blob/f662958b150a9d8d92fcbd0c9235e6bee1bedd67/aiohttp/helpers.py#L697 on the right loop, which if we have executed on a different loop, will raise an exception.
One way to fix this would be to make sure that we are running on the right event loop in _perform_refresh:
cloud-sql-python-connector/google/cloud/sql/connector/instance.py
Line 148 in de2852f
alternatively, we could try to schedule it properly here:
cloud-sql-python-connector/google/cloud/sql/connector/instance.py
Line 248 in de2852f
You could also try to make the argument that connectors should be 1:1 with event loops that they are being used on, but at the very least, I think we could add some type of check that we are on the correct event loop in
_perform_refresh
, especially given that things can actually work perfectly well and only fail later if multiple event loops are used.Furthermore, the connector class currently handles building a separate event loop if you do not provide one on which it should execute, which seems to strongly suggest that there is no expectation that you have to inject the loop.
I'm happy to implement a fix and I have several ideas about how to do so.
Example code (or command)
No response
Stacktrace
The text was updated successfully, but these errors were encountered: