lab.js icon indicating copy to clipboard operation
lab.js copied to clipboard

Bug with jumping to Sequences in Debug Plugin

Open jdpigeon opened this issue 1 year ago • 9 comments

In the current instantiation of the Debug Plugin, attempting to 'snapshot' a Sequence or a Screen nested within a sequence appears to break. What I've been noticing is that, on page reload, the study will jump to one node ahead of the target component, and then proceeds to the pinned component once the the first screen that is loaded ends.

jdpigeon avatar Aug 26 '22 16:08 jdpigeon

The issue seems to be related to the async update. If the fastforward is navigating to a root leaf, the fastforward operation will conclude and set tempLeaf before next() is triggered. However, if the fastforward navs to another iterator, another async will get kicked off, and next() will execute before that second promise returns and sets the correct tempLeaf

jdpigeon avatar Aug 26 '22 20:08 jdpigeon

This seems to bring this closer to a solution (https://github.com/FelixHenninger/lab.js/pull/190), although I'm still seeing some odd behaviour in my setup.

Doesn't it make sense to consider a Sequence only prepared when all of its nested sequences are prepared?

jdpigeon avatar Aug 26 '22 21:08 jdpigeon

Hej @jdpigeon , and many thanks for your report! I looked into this over the weekend, and am sorry to report that I can't figure out what's happening. I've attached a minimal study, and a brief video of how jumping works for me -- could you check if you're seeing the same behavior? Maybe this is browser-specific? I'd love to get to the bottom of this but I'm missing stuff so far, if you could provide a basic reproducible example, that would be fabulous.

Minimal demo study

(PS: I've been holding onto the new beta release until we figure this one out, let me know if you'd like me to revisit that)

FelixHenninger avatar Aug 29 '22 12:08 FelixHenninger

(also, which build are you on? I had a similar issue in e9640aa3e6a0961d94fa34acea3dc70f061ad3c2 , that's where I'd look for solutions)

FelixHenninger avatar Aug 29 '22 13:08 FelixHenninger

Ok, I can confirm that with a vanilla lab.js project the Debug Plugin does work as expected. I'm looking into what about my React-based implementation is leading to the unusual behaviour.

By the way, the problem appears to be specifically related to deeply nested Sequences. I was noticing that, when trying to fast-forward to a second or third-level nested Sequence, the next call would instead return the leaf node one index ahead of the target Sequence.

jdpigeon avatar Aug 29 '22 17:08 jdpigeon

Hi Dano, and thanks for your update! I'm wondering whether you might be able to replicate your issue with your debug tooling, but with 'vanilla' (deeply nested) HTML Screens? This has the (very vague) feel of a React issue I fought once. Happy to help debug if you can share a demo, or investigate if you figure out specifics, my hunch is that solving the issue will make the lab.js logic more robust too.

I'll cut a release over the course of the day, if that's alright, and would be happy to incorporate any further fixes. Best,

-Felix

FelixHenninger avatar Aug 30 '22 07:08 FelixHenninger

Hey Felix, it looks like this bug is actually due to a race condition between the prepare event and the run event of the top-level sequence. What was happening in our case was that the fastforward call was triggered by the prepare event but not allowed to complete before the run event. Updating our plugin to await the async onPrepare method (as is done in lab.js's Debug Plugin) fixed the issue, presumably by blocking the top-level sequence from completing its onPrepare method until the fastforward promise had resolved?

I'm still skeptical that there isn't a better way to handle the async nature of initializing iterators during a fastforward. It seems odd that a Plugin is able to determine the order in which a component prepares and runs. Do components await their Plugins' handle functions?

To help illustrate, here's the order of events I'm seeing without the await in the Plugin when trying to load a nested snapshot

  1. prepare event on top-level sequence
  2. top-level sequence initializes
  3. fastforward to inner sequence initiated
  4. extractIterator on inner sequence
  5. Unprepared component found, awaiting prepare()
  6. next is called, returning a leaf node from the top-level sequence
  7. top-level sequence proceeds to run
  8. Inner sequence finishes preparing
  9. fastforward completes

And here's the sequence with the await

  1. prepare event on top-level sequence
  2. top-level sequence initializes
  3. fastforward initiated
  4. Inner sequence encountered: extractIterator initiated
  5. Unprepared component found, awaiting prepare()
  6. Inner sequence finishes preparing
  7. fastforward completes
  8. next is called, returning the proper inner node of the nested sequence
  9. top-level sequence proceeds to run

The big difference is that next gets called before the preparation phase of the inner sequence completes.

The plugin is now working, but it might be nice to fix future async confusion. Would it make sense to have an on-going fastforward put the controller into an uninitialized state so that it has to wait before returning 'next'?

jdpigeon avatar Aug 30 '22 19:08 jdpigeon

Hi Dano,

thanks for getting to the bottom of this!

What was happening in our case was that the fastforward call was triggered by the prepare event but not allowed to complete before the run event.

Ok, that does make sense. Glad you figured it out!

I'm still skeptical that there isn't a better way to handle the async nature of initializing iterators during a fastforward.

I'm entirely with you here, and all ears for suggestions!

It seems odd that a Plugin is able to determine the order in which a component prepares and runs.

I'm not sure I follow here. In my recollection, the plugin only triggers fastForward on the controller, which handles the logic, all the while maintaining a consistent internal state. It needs to intercept the regular preparation process to do so; if the experiment is allowed to proceed normally, I think that the race condition you describe is a natural consequence -- the experiment will load and render as normal but at some point fastForwardwill have finished setting up a new component stack and a flip/jump will occur (the flash you describe above).

Do I understand you correctly that you're thinking about another mechanism to avoid race conditions outside of async/await? I'd love to learn, could you sketch out what behavior you would expect in the above scenario?

Do components await their Plugins' handle functions?

Yes, they do.

Ok, so much for now -- I'm glad this works for the moment, and happy to pick up the discussion around lab.js guts at the next opportunity!

-Felix

FelixHenninger avatar Aug 30 '22 22:08 FelixHenninger

Do components await their Plugins' handle functions?

Yes, they do.

Thanks for talking to time to parse all this! If it's true that Components await plugins, then that explains everything. With the Debug Plugin's handle function waiting for the fastForward Promise to complete, it'll block the Sequence from running until it's loaded.

I think this is a dangerous design, though. In my opinion, plugins should be considered "side effects" that execute on top of lab.js. It seems wrong that they can effect the internals of controller construction, especially with things that are as tricky to debug as async/await stuff.

In that case, my recommendation isn't to introduce something new beyond async/await, but to move the 'wait for fastforward to complete' logic into the controller itself. Like I mentioned before, maybe if the controller started fastforwarding, it could unset its initialized state so that 'flips' are temporarily paused

jdpigeon avatar Aug 30 '22 22:08 jdpigeon