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

Asyncio improvements #228

Open
2 of 9 tasks
Lash-L opened this issue Oct 28, 2024 · 0 comments
Open
2 of 9 tasks

Asyncio improvements #228

Lash-L opened this issue Oct 28, 2024 · 0 comments

Comments

@Lash-L
Copy link
Collaborator

Lash-L commented Oct 28, 2024

Place to keep track of all of the ideas @allenporter brought up in #222

  • making breaking changes to drop all the sync APIs and focus on simpler async APIs.
  • reduce the class hierarhcy so there are not multiple base classes involved, and use composition rather than inheritance.
  • adding logging for each outgoing request seq id and received resopnse seq id could help show the issue happening in more detail.
  • add guards to not send a message with a request id that has already been allocated. One example of a potential bug is that the choosing of a request id can pick an id that already exists since its just picking random numbers.
  • Add an explicit guard here when the future is already completed.
  • Add a guard in _async_response so that it doesn't overwrite an existing future if the request id is already in use
  • There are cases where a response is received and it checks against the queue and pulls out the message, but then if it doesn't already exist it continues anyway. Why would this ever be allowed to happen? seems like a bug or malformed response so it also needs logging rather than being ignored since it could hide concurrency bugs
  • Remove the multiple levels of futures happening between asyncio.ensure_future then a sync method _async_response which calls an async method _wait_response which directly awaits on a future. I don't think all these levels of indirection are needed
  • Don't pass around tuples of (response, exception). Just use the future built in exceptions
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

No branches or pull requests

1 participant