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

Remove QueuedReader and fix method cancellation #70

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

Conversation

tvoinarovskyi
Copy link
Contributor

As I stated in #52 fixing the cancellation issue without removing reader is too hard.
I've gone through the code base. The Reader is responsible for synchronous consumption of frames, but there is No need for that if we just maintain the code correctly:

  • Synchroniser.await should be called right after sender.send_* method. At least it should not contain yield from calls in between.

In asyncio we already have the loop, which has a great scheduling machinery, that will guaranty sequential call order.

@tvoinarovskyi
Copy link
Contributor Author

If it's hard to review for 0.5 we can just set it to be fixed in 0.6.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 96.009% when pulling bc4265f on TarasLevelUp:cancel_methods into 8f55f2f on benjamin-hodgson:master.

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