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

accept loop parameter for Queue #297

Closed
wants to merge 1 commit into from
Closed

Conversation

ali5h
Copy link

@ali5h ali5h commented Dec 1, 2020

What do these changes do?

Use case: If one wants to create the Queue in the main thread of the app, but use
it with an async loop that is running in a thread (not the one in the main thread).

Otw. one would have to create the queue in the thread to pick up the correct loop.

Are there changes in behavior for the user?

no change in behavior if used as before. Now user can pass a custom loop.

Checklist

  • I think the code is well written
  • Add a new news fragment into the CHANGES folder

This change is Reviewable

Use case: If one wants to create the Queue in the main thread of the
app, but use it with loop that running in a thread (not the one in the
main thread).

Otw. I have to create the queue in the thread to pick up the correct
loop.
@codecov
Copy link

codecov bot commented Dec 1, 2020

Codecov Report

Merging #297 (e9b4aee) into master (344ae2f) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #297   +/-   ##
=======================================
  Coverage   95.52%   95.52%           
=======================================
  Files           4        4           
  Lines        1274     1274           
  Branches       84       84           
=======================================
  Hits         1217     1217           
  Misses         50       50           
  Partials        7        7           
Flag Coverage Δ
unit 95.44% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
janus/__init__.py 99.68% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 344ae2f...e9b4aee. Read the comment docs.

@asvetlov
Copy link
Member

asvetlov commented Dec 1, 2020

Your request is error-prone, sorry.
The loop should not be transferred between threads, this is the rule of thumb.

@ali5h
Copy link
Author

ali5h commented Dec 1, 2020

then how do you want to run the queue in its own thread?

@asvetlov
Copy link
Member

asvetlov commented Dec 1, 2020

Create a queue instance in that thread than, that is what I can suggest

@ali5h
Copy link
Author

ali5h commented Dec 1, 2020

yes, that is exactly the issue, many of python async functions accept loop parameter, what is error prone about this?

@asvetlov
Copy link
Member

asvetlov commented Dec 1, 2020

In asyncio from Python 3.10 only two or three such public functions are left, all others were deprecated the loop argument by 3.8 and removed in 3.10

@ali5h ali5h closed this Dec 1, 2020
@ali5h
Copy link
Author

ali5h commented Dec 1, 2020

the problem is janus is asking for a running loop, which cannot be started yet.

@asvetlov
Copy link
Member

asvetlov commented Dec 1, 2020

The latest janus maybe doesn't fit your (not published) code, sure.
But I'm also sure that the code can be modified to satisfy the latest janus design.
Thus I cannot name it the "problem".

@brianbienvenu
Copy link

@ali5h Thanks for this snippet. I setup a new system with old Python 3.7 code & janus 0.4.0 on Python 3.8 & janus 0.6.1, and this was the only change which bit me. Patched the janus init.py with the suggested change and it works fine - good enough for another handful of years.

I'm sure this isn't best practice asyncio use for modern Python, but it's a lot lower friction than upgrading the rest of the codebase.

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.

None yet

3 participants