search

ekmett / machines

Networks of composable stream transducers
Other
340 stars 46 forks source link

Machines get stuck and don't stop #40

Closed greenrd closed 8 years ago

greenrd commented 9 years ago

This is an informal pull request, because I've created my commits against v0.4.1 in a new 0.4.1 branch which doesn't exist in your repo, so I can't create a proper pull request.

Please see https://github.com/greenrd/machines/tree/0.4.1

I have two commits there, one fixing a bug relating to repeatedly, and the other relating to fanout - both bugs cause machines to not stop when they should. Well, I'm sure the fanout one is a bug, anyway; I haven't actually verified that the repeatedly change fixes a bug, but it makes sense to me.

ekmett commented 9 years ago

The version of repeatedly that appears to have been live when you wrote those patches looks like it was just horribly horribly broken.

It should have had the same semantics as construct . forever all along.

ekmett commented 9 years ago

So I think the repeatedly changes are effectively made already.

The other is the change to fanout. For that I'm going to either defer to @acowley about the semantics of that operation (as he wrote it) or take it upon myself when I have more time and a clearer head.

greenrd commented 9 years ago

It seems I misunderstood the Haddock comment for repeatedly then. It says:

Generates a model that runs a machine until it stops, then start it up again.

But it doesn't start it up again:

Prelude Data.Machine Control.Applicative Control.Monad> take 10 <$> runT (repeatedly $ yield "hello" >> stop) :: IO [String]
["hello"]
ekmett commented 9 years ago

Ah. I see the description difference.

The word "stop" there came from before we added the Stop constructor!

repeatedly (yield "hello")

should run as intended. It really is intended to run until the plan returns at the end.

greenrd commented 9 years ago

OK, thanks, I understand now. I pushed some more commits. I avoided rewriting the git history because that might have been confusing.

acowley commented 9 years ago

The fanout change probably makes sense; I've not focused enough on machines that stop. It would be good to write some tests (doctests maybe) to demonstrate stopping behavior as it is a bit murky in most places. Or perhaps we need to do a documentation audit to standardize terminology so we can concisely say what happens when we, say, fanout before upstream stops, but downstream processes don't all stop at the same time.

ekmett commented 9 years ago

@acowley: Makes sense. Volunteering? ;)

bens commented 8 years ago

I just saw this ticket and thought I'd mention that #70 should fix the fanout part of it.

ekmett commented 8 years ago

With both repeatedly and fanout doing what they should I'm going to close this out. Please re-open if it is still an issue.