Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Redispatch hangs the browser #382

Closed
laurentsenta opened this issue Jul 25, 2017 · 8 comments
Closed

Redispatch hangs the browser #382

laurentsenta opened this issue Jul 25, 2017 · 8 comments

Comments

@laurentsenta
Copy link

Hi there, thanks for maintaining this repository, it's always a pleasure to work with re-frame & cljs.

This piece of documentation seems incorrect:
https://github.com/Day8/re-frame/blob/master/docs/Solve-the-CPU-hog-problem.md#why-does-a-redispatch-work
(It should be possible to dispatch an event from another one)

In the case of a simple infinite loop,
I'd expect 100% CPU usage AND the browser to be responsive. This is not the case: the browser / tab / console will hang until I kill it from the task manager.

Using a setTimeout solves the issue in this code.

Demo repo:

@danielcompton
Copy link
Contributor

I haven't had a chance to fully digest this, but it looks like you're not using ^:flush-dom?. If you read later on in the CPU hog docs you'll find an explanation of why this is necessary. Sorry if that's not the issue and we can take a deeper look, but at a glance that seems to be the problem?

@mike-thompson-day8
Copy link
Contributor

For the record, here's the docs on what should happen:
https://github.com/Day8/re-frame/blob/master/src/re_frame/router.cljc#L8-L61

@laurentsenta
Copy link
Author

laurentsenta commented Jul 26, 2017

@mike-thompson-day8 that's Double Buffering,
From the doc, we could have a simple tic | flush | toc | flush | tic | ... approach.
Two queues of events: one is being processed, the other is the destination of dispatch. They'd switch role between flush. No need for an FSM.

@danielcompton :flush-dom is useful if you want to FORCE a flush before processing an event. Say you enqueue an event that takes a few minutes to process, you want to give the browser a chance to clear all animations before.
In my case, I have thousands of 1ms events, by the doc, reframe should already give the control back to the browser.

@laurentsenta
Copy link
Author

laurentsenta commented Jul 26, 2017

I'd be happy to jump into the internals and provide a PR if it can make re-frame simpler.

@danielcompton
Copy link
Contributor

See binaryage/cljs-devtools#20 (comment) and d49965e. I think this might be what is happening? Can you try removing devtools and see if this still happens?

@danielcompton
Copy link
Contributor

@lsenta did this resolve your issue?

@laurentsenta
Copy link
Author

@danielcompton
thanks for sharing this, there's a small difference:

  • with devtools: the button is never released, it goes into highlighted state and the browser hangs,
  • without devtools: the animation completes

But the browser still hangs & the console goes unresponsive in both cases.

@mike-thompson-day8
Copy link
Contributor

Okay, I'm back to this issue after a long absence, sorry. When re-reading, it seems as if this issue is essentially @lsenta proposing:

It should be possible to dispatch an event from another one

It is a core assumption in re-frame that only one event at a time is handled. It makes everything every easy to reason about. That isn't going to change. It is a mistake to think of events like "function calls" where one can call another.

Closing.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants