Prevent infinite loops in lazily defined props #10879
Conversation
This prevents another kind of infinite loop in props with lazy getters, when real getter might try to access original property thus making it trigger `getValue` again. This is specifically the case 'regenerator-runtime', which happens only under some specific conditions (i.e. running in NodeJS inside `vm` context)
|
By analyzing the blame information on this pull request, we identified @yungsters to be a potential reviewer. |
|
/cc @MSLaguana |
facebook-github-bot
added
the
CLA Signed
| // This is specifically the case with 'regenerator-runtime' module, which | ||
| // checks if `global.regeneratorRuntime` property is already defined and | ||
| // thus triggers `getValue()` again. | ||
| valueSet = true; |
There was a problem hiding this comment.
This seems dangerous to set in the general case. It may work because Regenerator is written in a specific way but I don't think it's a good idea to mark the value as set when it hasn't been.
There was a problem hiding this comment.
Why it's dangerous? The getter is synchronous and nobody can access property's value from outside until get() call is completed. It's only possible to access it from 'inside', i.e. recursively, but in this case we'd get an infinite recursion, which we need to stop.
There was a problem hiding this comment.
It's dangerous in the recursive case because the recursively called code probably doesn't expect to receive undefined when it's called re-entrantly.
There was a problem hiding this comment.
So basically you suggest fixing this in regenerator? I'm fine with that, but i think this rings a bell. Honestly, I don't know whether it should be expected or not but I'd say that we should do something to stop recursion, because nobody can guarantee that at some moment we won't run into the same situation again.
There was a problem hiding this comment.
Fixing this in Regenerator is probably the best solution.
we should do something to stop recursion
Sure, the best solution I can think of is to throw an error with a clear message explaining which property was recursively accessed.
There was a problem hiding this comment.
I don't think that this particular issue is caused by regenerator though. When I saw this myself, it was actually the line https://github.com/facebook/react-native/blob/master/Libraries/Core/InitializeCore.js#L156 which caused the recursion: The access of global.regneratorRuntime called into the same getter function.
There was a problem hiding this comment.
@MSLaguana, you're right - I revisited this again and it looks like it doesn't related to regenerator - I can confirm that the loop starts in getter itself, not in regenerator.
Some experiments show that our problem has to do with Node - in particular it's very possible that we're getting into loop because delete global.regeneratorRuntime doesn't actually delete anything
When we're running this code in Chrome, after property deletion calling global.regeneratorRuntime doesn't trigger getter again because the property been deleted.
I think it's very likely that we're running into nodejs/node#6287 - not sure how to deal with this though
|
Closing this, as it doesn't seem to be the right solution |
This prevents another kind of infinite loop in props with lazy getters, when real getter might try to access original property thus triggering
getValueagain.This is specifically the case 'regenerator-runtime', which happens only under some specific conditions (i.e. running in NodeJS inside
vmcontext, see microsoft/vscode-react-native#340 for additional details)