This repository has been archived by the owner on Feb 12, 2022. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #2458.
This is a new version of my change in #2459. I closed that PR since @hermanventer had doubts which I confirmed after discovering my “fix” crashed my React Native internal bundle in a different way demonstrating its incorrectness.
After thinking about how to workaround this issue (since I was blocked on this) and fixing related issue #2466 I think I have a proper fix and I’m ready to defend it.
Note that this PR is based on top of #2466. The changes in this commit are the ones to look at.
The failing test case is the following. Annotated with some comments to help demonstrate the issue:
What is happening is that we modify three bindings in
f
before we throw aFatalError
.x
,y
, andz
. We also have threeJoinedNormalAndAbruptCompletions
since we’re using abstract booleans and throwing if the boolean is false. Our realm when we throw theFatalError
ends up looking like:When we throw the
FatalError
we want to restore all our bindings to a state before we started theevaluateForEffects()
call so that we can leave a residual call in pure scope. However, the way the code is currently written we don’t look intosavedCompletion.composedWith
even though there may be more bindings there. Instead we only look atsavedCompletion
so we only restore thez
binding.My previous fix modified
stopEffectCaptureAndUndoEffects()
to chasecomposedWith
, but that broke the contract ofstopEffectCaptureAndUndoEffects()
as @hermanventer mentioned. So instead I updated thefinally
block inevaluateForEffects()
which cleans up modified bindings of asavedCompletion
(in case we throw in the middle of evaluation) to more explicitly clean up the bindings of the saved completion and all composed completions. This preserves and fixes our effect cleanup behavior without changing the meaning ofstopEffectCaptureAndUndoEffects()
.@hermanventer I know you mentioned you wanted to look at this issue, sorry if I’m stepping on your toes. Since I was blocked I thought a bit more about this issue and figured I was directionally correct, but the specific fix in #2459 was wrong. Also sorry for the PR churn. Thought I wasn’t going to come back to this issue.