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

Mithril stream.end(true) ends other dependent streams as well #2601

Closed
dontwork opened this issue Jun 2, 2020 · 12 comments · Fixed by #2603
Closed

Mithril stream.end(true) ends other dependent streams as well #2601

dontwork opened this issue Jun 2, 2020 · 12 comments · Fixed by #2603
Assignees
Labels
Type: Bug For bugs and any other unexpected breakage

Comments

@dontwork
Copy link

dontwork commented Jun 2, 2020

**Mithril version: 2.0.4

Browser and OS: Windows 10, Chrome Version 83.0.4103.61

Project:

mithril stream vs flyd stream

credit to @foxdonut for the reproduction

notice in the mithril example how o2 doesn't receive the false value
but using the same example with flyd, o2 does receive the false value

The code I was writing was this:

    if (auth.refreshing()) {
      const waiting = auth.refreshing.map((val) => {
        if (val === false) {
          resolve(token())
          waiting.end(true)
        }
      })
    }

The unexpected behaviour i experience is that when there were X numbers of waiting streams the code waiting.end(true) seemed to end ALL of them

@dontwork dontwork added the Type: Bug For bugs and any other unexpected breakage label Jun 2, 2020
@dead-claudia
Copy link
Member

Could you provide a fuller repo of this? Ideally a Flems? It'd be much easier to figure out if it's by design or a proper bug - every stream library's behavior is just the sum of a ton of subtlety.

@foxdonut
Copy link

foxdonut commented Jun 8, 2020

@isiahmeadows the two links above ("mithril stream" and "flyd stream") are Flems showing the issue.

@dead-claudia
Copy link
Member

Yeah, that's a nasty bug. o1 and o2 should either both end or both continue. I'll look into it.

Didn't pay close attention to the links. Sorry!

@dead-claudia
Copy link
Member

Just for clarity, here's a minimal example that doesn't involve nesting: link

@foxdonut
Copy link

foxdonut commented Jun 8, 2020

No worries!

The minimal example doesn't expose the bug though... does it?

@dead-claudia
Copy link
Member

@foxdonut It prints the same thing as your Mithril link. I just removed some unnecessary printing.

@foxdonut
Copy link

foxdonut commented Jun 9, 2020

@isiahmeadows Please have a closer look because it's not the same thing and it does not expose the bug. In my Mithril link, notice that o2 doesn't receive the false value, only o1. That is the bug. In the Flyd link, both o1 and o2 receive the false value.

@dead-claudia
Copy link
Member

Oh, whoops. You're right, my bad. 🤦‍♀️

@jaydh
Copy link

jaydh commented Jun 14, 2020

I'm pretty sure this has to do with ending a stream within a dependent stream's map. What's really going is better illustrated here. Note that after o1 gets ended, o2 is missing but o3-5 show results. This is because as the parent stream is executing, the end call unregisters the child and splices the dependent streams/dependent functions during the iteration which causes the skip.

I agree this isn't ideal behavior and probably the simple fix is to mark stream removal, check for stream removal before fn execution, unregister all marked for removal streams after the execution calls.

Thoughts?

@dead-claudia
Copy link
Member

@jaydh The actual issue is much simpler: it's skipping all remaining children, even ones that weren't ended, even though they remain subscribed - note that in the subsequent emit after that one, it's still received by both streams. I suspect a return is used where a continue or similar should be used instead.

@jaydh
Copy link

jaydh commented Jun 14, 2020

@isiahmeadows That is not what is happening here. Note that after o1 gets ended in the execution of o2, o3 gets skipped, but o4 and o5 still print results.

This behavior is explained in the source, when invoking refreshing(false) we have the parent stream with

dependentStreams = [o1, o2, o3, o4, o5]
and we run through

dependentStreams.forEach(function(s, i) { s(dependentFns[i](value)) })
o1 runs (index 0),
o2 runs (index 1), but now we call _unregisterChild(o1) which mutates depedentStreams to look like [o2, o3, o4, o5]
and now we're on index 2, which now points to o4 which is why o3 gets skipped and o5 still prints.

@dead-claudia
Copy link
Member

Now that I look closer, that's even weirder.

dead-claudia added a commit to dead-claudia/mithril.js that referenced this issue Jun 16, 2020
dead-claudia added a commit to dead-claudia/mithril.js that referenced this issue Jun 16, 2020
@dead-claudia dead-claudia mentioned this issue Jun 16, 2020
11 tasks
dead-claudia added a commit that referenced this issue Jun 16, 2020
Fix issue where ending a stream in the middle of a stream callback would result in erroneous parent stream state for the rest of that emit.
StephanHoyer pushed a commit that referenced this issue May 16, 2022
Fix issue where ending a stream in the middle of a stream callback would result in erroneous parent stream state for the rest of that emit.
@dead-claudia dead-claudia moved this to Closed in Triage/bugs Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug For bugs and any other unexpected breakage
Projects
Status: Closed
Development

Successfully merging a pull request may close this issue.

4 participants