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

Jump to bottom

Bug: Normal update between Idle render and a Ping causes Fallback to get stuck #18657

Closed
gaearon opened this issue Apr 17, 2020 · 0 comments · Fixed by #18663
Closed

Bug: Normal update between Idle render and a Ping causes Fallback to get stuck #18657

gaearon opened this issue Apr 17, 2020 · 0 comments · Fixed by #18663
Labels
Component: Concurrent Features Type: Bug

Comments

@gaearon
Copy link
Collaborator

gaearon commented Apr 17, 2020 •
edited
Loading

Repro case with a master build: https://codesandbox.io/s/stoic-mcnulty-dhygf?file=/src/App.js:668-703

Expected: we see content after a second.
Actual: fallback never resolves.

This happens in a sequence of:

  1. Normal update
    • which suspends
  2. Idle update
    • which also suspends
  3. An unrelated normal update immediately followed by a ping (so they're batched)
    • here, we decide to stay on fallback, but should've shown the content

If you remove _setVersion(v => v + 1); on line 44 then the issue goes away.
@gaearon gaearon added Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug Component: Concurrent Features Type: Bug and removed Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug labels Apr 17, 2020
acdlite added a commit to acdlite/react that referenced this issue Apr 17, 2020
@acdlite
Failing test for facebook#18657
b7a0583
@acdlite acdlite mentioned this issue Apr 17, 2020
Merged
acdlite added a commit that referenced this issue Apr 17, 2020
@acdlite
Fix for #18657 (#18663)
cfefc81 
* Failing test for #18657

* Remove incorrect priority check

I think this was just poor factoring on my part in #18411. Honestly it
doesn't make much sense to me, but my best guess is that I must have
thought that when `baseTime > currentChildExpirationTime`, the function
would fall through to the
`currentChildExpirationTime < renderExpirationTime` branch below.

Really I think just made an oopsie.

Regardless, this logic is galaxy brainéd. A goal of the Lanes refactor
I'm working on is to make these types of checks -- is there remaining
work in this tree? -- a lot easier to think about. Hopefully.
@gaearon gaearon mentioned this issue Apr 18, 2020
Closed
dubzzz added a commit to dubzzz/react that referenced this issue Apr 23, 2020
@dubzzz
Detect facebook#18657 thanks to fuzzing
2c1f191 
The current commit is totally work in progress but it already found back the issue by reporting the following counterexample:
```
[Scheduler`
      -> [task#2] sequence::Scheduling "8" with priority 3 resolved
      -> [task#1] promise::Request for "447b0ed" resolved with value "resolved 447b0ed!"`,"447b0ed",[{"priority":3,"text":"8"}],<function :: ["447b0ed"] => true, ["8"] => true>]
```
Reproduced by https://codesandbox.io/s/strange-frost-d4ujl?file=/src/App.js

Related to facebook#18669
dubzzz added a commit to dubzzz/react that referenced this issue Apr 23, 2020
@dubzzz
Detect facebook#18657 thanks to fuzzing
45b9398 
The current commit is totally work in progress but it already found back the issue by reporting the following counterexample:
```
[Scheduler`
      -> [task#2] sequence::Scheduling "8" with priority 3 resolved
      -> [task#1] promise::Request for "447b0ed" resolved with value "resolved 447b0ed!"`,"447b0ed",[{"priority":3,"text":"8"}],<function :: ["447b0ed"] => true, ["8"] => true>]
```
Reproduced by https://codesandbox.io/s/strange-frost-d4ujl?file=/src/App.js

Related to facebook#18669
dubzzz added a commit to dubzzz/react that referenced this issue Apr 23, 2020
@dubzzz
Add fuzzing tests to React.Suspense
07b7869 
It would actually have detected the issue facebook#18657.

It found the following counterexample:
```
[Scheduler`
      -> [task#2] sequence::Scheduling "8" with priority 3 resolved
      -> [task#1] promise::Request for "447b0ed" resolved with value "resolved 447b0ed!"`,"447b0ed",[{"priority":3,"text":"8"}],<function :: ["447b0ed"] => true, ["8"] => true>]
```
Reproduced by https://codesandbox.io/s/strange-frost-d4ujl?file=/src/App.js

Related to facebook#18669
# for free to join this conversation on GitHub. Already have an account? # to comment
Assignees
No one assigned
Labels
Component: Concurrent Features Type: Bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix for #18657 acdlite/react
1 participant
@gaearon