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

fanchat as unit test #46

Merged
merged 11 commits into from
Jan 18, 2016
Merged

fanchat as unit test #46

merged 11 commits into from
Jan 18, 2016

Conversation

glyph
Copy link
Member

@glyph glyph commented Dec 8, 2015

this isolates the problem away from the full integration stack,
because we are not touching Twisted at all here.

Review on Reviewable

this isolates the problem away from the _full_ integration stack,
because we are not touching Twisted at all here.
glyph added 6 commits December 8, 2015 01:27
plus missing parameters
not *entirely* sure why this does fix it, but it fixes an inconsistency
between different states; _pauseBecausePauseCalled should not be None
when we are paused regardless of whether we are in fact holding a pause
for an upstream fount or not.
there's no drain on the end, of course it should be paused.
@glyph
Copy link
Member Author

glyph commented Dec 8, 2015

The problem here might actually be more fundamental than a bug.

fanchat has an explicit flow-control cycle in it: the participant's router feeds into all the chat rooms that they're participating in; in turn, each chat room broadcasts back into the participant's router.

The router is itself a fan.Out underneath, and conceptually as well, because the router could produce output to all of its downstreams in response to one input from its upstream.

We don't get an infinite loop because Pauser explicitly prevents reentrancy of pausing, but once any of these have been paused, it will remain paused forever.

It might be nice to have some kind of cycle detection so that you get an error if you try to set this up without appropriate precautions.

The right solution here would probably be to implement backpressure policy, specifically #37. If we had a Roll somewhere in the cycle, with the ability to buffer even a single element, that ought to un-stick the pause loop, at least at the start. I will need to think a bit about what should happen when that buffer fills up though...

glyph added 2 commits December 8, 2015 03:07
'joined' and 'spoke' messages from Channels are fully cooked anyway; no
need to push them through the Participant for command-processing.
@glyph
Copy link
Member Author

glyph commented Dec 8, 2015

OK, we could also just eliminate the cycle, which is apparently unnecessary and redundant.

I have no idea why this shows coverage dropping. We should really switch to codecov, which seems to have fewer random bugs.

@glyph
Copy link
Member Author

glyph commented Jan 18, 2016

Reviewed 3 of 4 files at r1, 1 of 1 files at r2, 4 of 4 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from the review on Reviewable.io

glyph added a commit that referenced this pull request Jan 18, 2016
fanchat as unit test - fix some flow control bugs
@glyph glyph merged commit 8522684 into master Jan 18, 2016
@glyph glyph deleted the fanchat-as-test branch January 18, 2016 03:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant