-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[V2] Refactor sender #4484
base: v2
Are you sure you want to change the base?
[V2] Refactor sender #4484
Conversation
Please motivate or explain the changes further. It's a bit difficult to get an overview on what the goal is. |
Sender is not working properly, it sometimes raises RuntimeError("got response for unsent request") and bad msg id, I tried to solve request queuing using queue |
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.
If I'm reading correctly, this seems to tie requests and responses way too tightly. The library should be free to send as many requests as it wants, and have responses received in any order.
request = await self._requests.get() | ||
await self._try_fill_write(request) |
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.
Network should be able to run even if there are no requests to make. This will block until there is one which does not seem correct.
_mtp_buffer=bytearray(), | ||
_requests=[], | ||
_request_event=Event(), | ||
_requests=Queue(1), |
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.
I'm not sure why there's a limit of a single request. This does enable backpressure, but it might make it harder for the library to be able to combine multiple requests into a single container.
There is one todo, the branch is not ready yet, but the main part is ready for review