AsyncioConnection: fix initialize_reactor when called in event loop #327
Conversation
Previously, if executed within existing asyncio loop, driver would take the loop, assume it's not used and start it in a separate thread. Additionally, if executed outside of loop, driver would create a new one and make it default for calling thread. Those behaviors are wrong so they are changed. Now driver creates its own loop and executes it in a thread. Code that handled pid changes, which can happen when class is transferred using e.g. multiprocessing, is fixed too - previously it didn't create new thread after such transition.
|
How did you test this ? We desperately need a test that works with asyncio, in a similar fashion as test.py Anyhow we still shouldn't make asyncio as a fallback, and align to upstream behavior until we have a clear plan of how to test and properly validate this event loop mode |
Manually:
I can add a test for this case, it shouldn't be too hard
I think I disagree. All integration tests passed with asyncio, it has the advantage of not requiring additional binary dependencies, and I think we are unaware of any other bugs. |
so lets try
that's exactly the problem, it's not battle proven enough yet (not by us, and not by datastax) I still think we should revert that call, and align with upstream, it too risky and it would backfire on our core unit tests first, and on user and customer right after, the integration suite doesn't really cover it enough.
|
Ok, I'll try to add some tests tomorrow.
So let's make it more battle tested and fix issues that we find. How to run dtests with asyncio backend?
|
patching this place, would do the trick:
|
|
running gating tests with it (it's a big portion of all of the dtest) |
|
we can install this PR into dtest like that: and dtest pipeline can use that trick, with the following parameter I've send out a gating run with this PR:
it probably not gonna teach us anything new, since dtest isn't using asyncio on it's end. @roydahan @yaronkaikov it would be nice if we can apply similar trick to test.py and byo pipelines |
|
Sounds good to me. |
something along the lines of: DRIVER_INSTALLATION_CMD=pip install git+https://github.com/Lorak-mmk/python-driver.git@fix-asyncioconnection
./dbuild bash -c "${DRIVER_INSTALLATION_CMD}; test.py ..."and away to pass from the pipeline arguments the actual version up into the command it self I did it manually once with test.py, but it was from local repo, so I need to add a mount point. |
|
@Lorak-mmk result from dtest, show the exactly same SSL related issue we seen before this fix. so as expected, it didn't told us anything new about if this fix is good enough. I think one might want to test it with test.py, if you didn't come up with build a specific test for it. |
|
Who owns the action item here? We need to see it solved! |
It's not that high priority now - issue in Scylla was solved by publishing ARM wheels, and I think we are going to mark asyncio backend as experimental anyway until #330 is solved. |
Previously, if executed within existing asyncio loop, driver would take the loop, assume it's not used and start it in a separate thread.
Additionally, if executed outside of loop, driver would create a new one and make it default for calling thread.
Those behaviors are wrong so they are changed. Now driver creates its own loop and executes it in a thread. Code that handled pid changes, which can happen when class is transferred using e.g. multiprocessing, is fixed too - previously it didn't create new thread after such transition.