Skip to content
This repository has been archived by the owner on Sep 13, 2018. It is now read-only.

Check for space in frame_buf #161

Closed
wants to merge 1 commit into from
Closed

Check for space in frame_buf #161

wants to merge 1 commit into from

Conversation

ZoeyR
Copy link

@ZoeyR ZoeyR commented Apr 23, 2017

Closes #143

Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. Looks good. I asked for some additional clarification on a line below.

It would also be nice to add some tests for these cases.

@@ -304,7 +304,9 @@ impl<T> Multiplex<T> where T: Dispatch {
while self.run {
// TODO: Only read frames if there is available space in the frame
// buffer
if let Async::Ready(frame) = try!(self.dispatch.get_mut().inner.transport().poll()) {
if self.frame_buf.full() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been a while since I dealt with this code. When the frame_buf has capacity again, something has to notify the task so that it starts running again. Could you explain how this happens?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to admit I'm not as familiar with some of the inner workings as I would like, but this is how it works as far as I can tell. If the buffer is at capacity that necessarily means that there is data to flush. If the flush needs to block that is when the task will be parked. Once the flush is finished the task will start running again. If the flush did not need to block then the task will continue running through another loop of dispatch, read, write, flush.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is plausible... a test would help confirm it :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have to figure out how to write a small test that can trigger a filled buffer case consistently. I was able to trigger it on a large project of mine but that doesn't readily translate to a good unit test.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I suspected, writing a small test that can consistently trigger this edge case is far from trivial. I'm in the middle of finals right now so I will not be able to get around to writing a unit test for this for a week or so

@ZoeyR
Copy link
Author

ZoeyR commented Apr 26, 2017

Well that's odd, the commits I squashed together came back. I'll have to go back and fix that now.

@ZoeyR ZoeyR closed this by deleting the head repository Mar 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants