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

io.asyncioreactor: fix deprecated usages for working with python>=3.10 #271

Merged
merged 5 commits into from
Nov 19, 2023

Conversation

fruch
Copy link

@fruch fruch commented Nov 9, 2023

  • 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

@fruch fruch requested a review from Lorak-mmk November 9, 2023 10:35
* 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
@fruch fruch force-pushed the fix_for_asyncio branch 2 times, most recently from cb2b29e to 9a2428c Compare November 13, 2023 12:55
there are some places were we don't need to pass or create
the asyncio loop, and we should avoid it
@fruch fruch merged commit bc5cf17 into scylladb:master Nov 19, 2023
18 of 20 checks passed
Copy link

@Lorak-mmk Lorak-mmk left a comment

Choose a reason for hiding this comment

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

I'm a bit surprised by this merge - I did not review it yet, but I reviewed the previous version, before the split, made some comments and one imo important comment was neither fixed nor responded to.
#260 (comment) - you still call asyncio.set_event_loop(cls._loop) (but now conditionally for some reason) and I still don't see how it makes sense in any scenario.

Also, I proposed an alternative way of dealing with loop argument, and it can be made prettier than the example I posted in previous review, by having sync and async part of init, something like that:

    async def _init_async(self):
        self._write_queue = asyncio.Queue()
        self._write_queue_lock = asyncio.Lock()

and in init call asyncio.run_coroutine_threadsafe(self._init_async(), loop=self._loop).result() in a place where queue and lock were previously initialized.
This has the advantage of not using deprecated arguments ever, so less warnings from driver. And then another one of my comments can be applied: #260 (comment)

Choose a reason for hiding this comment

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

In push function, self._loop.create_task is called and it's return value is ignored. While the tests may pass now, this code is not correct and this example is called out in docs as a source of bugs: https://docs.python.org/3/library/asyncio-task.html#asyncio.create_task

@fruch
Copy link
Author

fruch commented Nov 19, 2023

I'm a bit surprised by this merge - I did not review it yet, but I reviewed the previous version, before the split, made some comments and one imo important comment was neither fixed nor responded to. #260 (comment) - you still call asyncio.set_event_loop(cls._loop) (but now conditionally for some reason) and I still don't see how it makes sense in any scenario.

Taking it completely won't always work, there are cases it can be call while no event loop is define, you can remove it and see it failing the tests

Also, I proposed an alternative way of dealing with loop argument, and it can be made prettier than the example I posted in previous review, by having sync and async part of init, something like that:

    async def _init_async(self):
        self._write_queue = asyncio.Queue()
        self._write_queue_lock = asyncio.Lock()

and in init call asyncio.run_coroutine_threadsafe(self._init_async(), loop=self._loop).result() in a place where queue and lock were previously initialized. This has the advantage of not using deprecated arguments ever, so less warnings from driver. And then another one of my comments can be applied: #260 (comment)

Yeah it might remove the warning, but I just understood what you meant

One still need to get it tested

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.

2 participants