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

infinite recursion fix #38

Merged
merged 1 commit into from
Feb 10, 2016

Conversation

davidmarkclements
Copy link
Contributor

Infinite recursion has been identified with the select event, but it may occur else where.

Here's the scenario:

  • I have a list element with an onSelectItem handler that triggers a redux action
  • this in turn triggers a react update lifecycle
  • during the cycle, the list element re-renders
  • part of blesseds rendering process is to call select on each of the items in the list as they are added
  • this triggers N select events (where N is the amount of items in list)
  • each of this triggered events onSelectItem to be triggered, firing a redux action with a new payload
  • so for every item in the list, an update cycle is started
  • blessed calls select on all of the items in all of the new update cycles and so on (N to the power of N)
  • consequently the process crashes

If we block events during an update cycle, this solves the problem - since the update cycle is synchronous this shouldn't block any actual user events (... right?)

@Yomguithereal
Copy link
Owner

The weird part here is:

part of blesseds rendering process is to call select on each of the items in the list as they are added

Why is that?

@davidmarkclements
Copy link
Contributor Author

Not sure - it may a sort of lazy way to do some kind of calculation - add the component, select it, and then determine information from that (maybe). From what I've seen there's a couple of points in the blessed codebase that follows this sort of approach (possibly because there's differences between terminal environments so the only way to know something for sure (like an exact dimension or something) is to actually paint the element).

@Yomguithereal
Copy link
Owner

Doesn't this fix #45 also?

@davidmarkclements
Copy link
Contributor Author

as far as I can recall - you need both to get a complete fix

Yomguithereal added a commit that referenced this pull request Feb 10, 2016
@Yomguithereal Yomguithereal merged commit 8dce749 into Yomguithereal:master Feb 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants