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

Fixes to stabilize asyncioreactor for multiple versions of python #1189

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

fruch
Copy link

@fruch fruch commented Nov 13, 2023

few fixes to stabilize asyncio reactor

also few fixes to the tests, to ignore warning correctly, and skip a test which doesn't work for asyncio

* stop using the loop argument for `asyncio.Lock` and asyncio.Quoue`
* on the lock replace `with await` with `async with`, which is the correct
  syntax for using that lock
since python3.8 we have this warning:
```
DeprecationWarning('The loop argument is deprecated since Python 3.8, and scheduled for removal in Python 3.10.')
```

and it's o.k. to have it since on Python 3.10 and up, we stop using that argument
asyncio can't do timeouts smaller than 1ms, as this test requires
it's a limitation of `asyncio.sleep`

Fixes: scylladb#263
there are some places were we don't need to pass or create
the asyncio loop, and we should avoid it
@fruch
Copy link
Author

fruch commented Nov 13, 2023

@absurdfarce FYI

as for CLA, I'm need to sign it ? or it was enough that someone already sign on our behalf ?

@absurdfarce
Copy link
Collaborator

absurdfarce commented Jan 18, 2024

Hey @fruch, apologies for (again) taking so long to get back to you; as usual I'm trying to put out too many fires at one time.

I'd like to make stabilization of the asyncio reactor one of the main features for the next driver release (likely 3.30) with the goal of making it the default reactor, if not for all versions then at least for Python 3.12 and up. I've filed PYTHON-1375 to describe that work with a comment specifically mentioning the work you guys have done and this PR.

More to come on this: I'm still juggling a ton of things (again as usual :( ) so I'm not sure what the timing looks like on this. But I'd definitely like to get it out sooner rather than later.

Oh, also, as for the CLA... you don't need to worry about it. There's a company-wide CLA for Scylla so you should be fine.

@fruch
Copy link
Author

fruch commented Jan 18, 2024

Hey @fruch, apologies for (again) taking so long to get back to you; as usual I'm trying to put out too many fires at one time.

I'd like to make stabilization of the asyncio reactor one of the main features for the next driver release (likely 3.30) with the goal of making it the default reactor, if not for all versions then at least for Python 3.12 and up. I've filed PYTHON-1375 to describe that work with a comment specifically mentioning the work you guys have done and this PR.

More to come on this: I'm still juggling a ton of things (again as usual :( ) so I'm not sure what the timing looks like on this. But I'd definitely like to get it out sooner rather than later.

Oh, also, as for the CLA... you don't need to worry about it. There's a company-wide CLA for Scylla so you should be fine.

No worries, it's exactly the same here 😀

if sys.version_info[0] == 3 and sys.version_info[1] < 10:
loop_args['loop'] = self._loop
self._write_queue = asyncio.Queue(**loop_args)
self._write_queue_lock = asyncio.Lock(**loop_args)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This came up when we were working on PYTHON-1313. Docs I found at the time indicated that since Python 3.7 the event loop had been derived from the current threads event loop and the "loop" param was essentially ignored. We're currently looking at 3.8 through 3.12 so this block only applies to Python 3.8.x and 3.9.x. Do you think it's worth it to continue to pass a "loop" param for those cases?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it seems to be needed, I.e. taking it down got it broke.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@absurdfarce
Copy link
Collaborator

Hey @fruch , with any luck I've been able to (kind of) clear off my calendar and get back to working on the Python driver again. I'm working through the changes you guys have made here so this will be something like a slow-motion review; I want to make sure I really understand everything that's in here.

cls._loop = asyncio.new_event_loop()
asyncio.set_event_loop(cls._loop)
try:
cls._loop = asyncio.get_running_loop()
Copy link
Collaborator

@absurdfarce absurdfarce May 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 get_running_loop() is preferred as the entry point over get_event_loop() per asyncio docs. No obvious reason it wouldn't be preferred to new_event_loop() + set_event_loop() as well.

Copy link

@aisven aisven Jul 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@absurdfarce turns out that in the meantime in a fork this part was refined ⚠️

see code change at

https://github.com/scylladb/python-driver/pull/327/files#diff-932c40bc0835ee500494816d5cfa78f820240c9265c6582cd789356ea56f6447R124

and respective explanation at

scylladb#327 (comment)

@fruch
Copy link
Author

fruch commented May 28, 2024

Hey @fruch , with any luck I've been able to (kind of) clear off my calendar and get back to working on the Python driver again. I'm working through the changes you guys have made here so this will be something like a slow-motion review; I want to make sure I really understand everything that's in here.

FYI, we found issues with the asyncio implementation, the tests we have here in the integration suite are not using asyncio on their own.

If you can apis from within a core routine, it's not really gonna work.

So we do need asyncio based tests, cause that option isn't covered

absurdfarce added a commit that referenced this pull request May 29, 2024
@absurdfarce
Copy link
Collaborator

absurdfarce commented Jun 27, 2024

Thanks for letting me know @fruch. Are you saying you've found issues with the impl of asyncio in Python or with what's in the driver's asyncio reactor?

I've been doing some testing locally and I'm seeing some issues around SSL ops that have me a bit worried. I'm trying to isolate what I'm seeing by bringing test_ssl back from the dead (see PYTHON-1372) but that's been slower going than I expected... especially when there's lots of firefighting going on as well.

@fruch
Copy link
Author

fruch commented Jun 27, 2024

Thanks for letting me know @fruch. Are you saying you've found issues with the impl of asyncio in Python or with what's in the driver's asyncio reactor?

I've been doing some testing locally and I'm seeing some issues around SSL ops that have me a bit worried. I'm trying to isolate what I'm seeing by bringing test_ssl back from the dead (see PYTHON-1372) but that's been slower going than I expected... especially when there's lots of firefighting going on as well.

Yes SSL seems to be broken with asyncio

Also what are you doing cqlsh, it uses source only driver, when packaged ? so python 3.12 kind of leaves you without eventloop

@aisven
Copy link

aisven commented Jul 20, 2024

@absurdfarce First of all thank you for your work. I want to ask, is this interesting PR thanks to @fruch here about to land in a release soon? I looked over the changes and saw that the changes to production code are not that many, and mostly regarding handling different python versions. Much of this PR is about improving some tests. I'd like to see a release with this.

@@ -173,7 +180,7 @@ def push(self, data):

async def _push_msg(self, chunks):
# This lock ensures all chunks of a message are sequential in the Queue
with await self._write_queue_lock:
async with self._write_queue_lock:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 async with seems to be the correct syntax to use that lock in terms of an asyncio context manager.

@@ -162,7 +169,7 @@ def push(self, data):
else:
chunks = [data]

if self._loop_thread.ident != get_ident():
if self._loop_thread != threading.current_thread():
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 Since self._loop_thread != threading.current_thread() is a common and direct way to verify that the current thread is not the specified thread, no matter how the thread idents are typed, created, stored, updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants