-
Notifications
You must be signed in to change notification settings - Fork 310
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
Make ChannelQueue.get_msg true async #892
Make ChannelQueue.get_msg true async #892
Conversation
Codecov Report
@@ Coverage Diff @@
## main #892 +/- ##
==========================================
+ Coverage 70.28% 70.44% +0.16%
==========================================
Files 65 65
Lines 7683 7698 +15
Branches 1284 1287 +3
==========================================
+ Hits 5400 5423 +23
+ Misses 1896 1887 -9
- Partials 387 388 +1
Continue to review full report at Codecov.
|
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good and make sense - thank you @CiprianAnton!
@meeseeksdev please backport to 1.x |
…#893) Co-authored-by: Ciprian Anton <ciprian.anton@ni.com>
ChannelQueue.get_msg is marked as async, however it's a blocking call.
Looking at https://github.com/jupyter/nbclient/blob/main/nbclient/client.py#L806, there is a call to
get_msg
which may block infinite. Now that code is fine, because the coroutine is being run as a background task and timeout being handled with asyncio.However, thinking about a little bit, get_msg will block the entire thread, not giving a chance to other tasks to run.
We can easily make this method true async. I also considered asyncio.Queue, but that is not thread safe and we need it for recv_thread.