fix "RuntimeError: This event loop is already running"

[temichus]

fix an issue that may occur in asyncioreactor.py when cls._loop = asyncio.get_running_loop() returns true and then call ls._loop.run_forever that raise exception:

"RuntimeError: This event loop is already running"

[temichus]

from cassandra.io.asyncioreactor import AsyncioConnection
import asyncio
import time
from threading import Lock, Thread, get_ident

# test 1 
def test():
    AsyncioConnection.initialize_reactor()

test() # work without problems in synchronous Python

# test 2
async def await_test():
    AsyncioConnection.initialize_reactor()

loop = asyncio.get_event_loop()

asyncio.run_coroutine_threadsafe(
                await_test(),
                loop=loop
            )

loop_thread = Thread(target=loop.run_forever,
                                          daemon=True, name="asyncio_thread")
loop_thread.start() # RuntimeError: This event loop is already running

# test 3 
asyncio.run(await_test())  # RuntimeError: This event loop is already running
time.sleep(5) # sleep to wai output fail from Thread

[Lorak-mmk]

Hmm... I see a problem with this approach: A user can create a Session on a thread with event loop, then stop the event loop and use session from the same / other thread. I think the safest solution is my first proposed one: don't try to use user's loop, always create our own loop and thread.

[fruch]

Hmm... I see a problem with this approach: A user can create a Session on a thread with event loop, then stop the event loop and use session from the same / other thread. I think the safest solution is my first proposed one: don't try to use user's loop, always create our own loop and thread.

cloud be, anyhow this kind of show the lack of coverage we have, testing wise for the possibility of using it. maybe the call of the upstream to default to error and not asyncio was a better call.

I think we need a bit more investment in unit/integration tests for the asyncio case and also the usage of driver from async code, as scylla core test.py is doing

[Lorak-mmk]

That's definitely true, this driver has a lot of bugs, terrible codebase and not enough tests, properly fixing things is extremely hard. I wonder how much effort would it take to take https://github.com/Intreecom/scyllapy , implement missing features so it has ~parity with other drivers and move dtests / sct to it.

For the time being I'd still advocate for the solution I described (always create event loop and thread), so that the current issue can be fixed while minimizing the risk of new breakage.

[fruch]

That's definitely true, this driver has a lot of bugs, terrible codebase and not enough tests, properly fixing things is extremely hard. I wonder how much effort would it take to take https://github.com/Intreecom/scyllapy , implement missing features so it has ~parity with other drivers and move dtests / sct to it.

This a nice one, I didn't know someone took this thing forward.

But it has only async apis, porting dtest to it, would be quite a nightmare, and would quite a while also other tools need to be ported, like cqlsh and such, if we really want to stop supporting this driver completely

For the time being I'd still advocate for the solution I described (always create event loop and thread), so that the current issue can be fixed while minimizing the risk of new breakage.

as I said, might be o.k., but doing so with no test what so ever, would be doing it blind fold. (like we were doing so far)

[temichus]

@Lorak-mmk @fruch what is the decision for this?

[fruch]

@Lorak-mmk @fruch what is the decision for this?

No decision yet, but it's clear we'll need to invest more in testing around asyncio usage, before getting this one merged (or an equivalent fix as @Lorak-mmk pointed to)

Just to confirm, it's not a blocker for you right ? (I.e. you manged to get the libev event loop working for you?)

[temichus]

@Lorak-mmk @fruch what is the decision for this?

No decision yet, but it's clear we'll need to invest more in testing around asyncio usage, before getting this one merged (or an equivalent fix as @Lorak-mmk pointed to)

Just to confirm, it's not a blocker for you right ? (I.e. you manged to get the libev event loop working for you?)

waiting for confirmation from @tchaikov in https://github.com/scylladb/scylladb/issues/16676

[temichus]

close according to https://github.com/scylladb/scylladb/issues/16676#issuecomment-2003599413