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

Bug: Some transition updates haven't been rendered #24350

Closed
zh-lx opened this issue Apr 12, 2022 · 10 comments
Closed

Bug: Some transition updates haven't been rendered #24350

zh-lx opened this issue Apr 12, 2022 · 10 comments
Labels
React 18 Bug reports, questions, and general feedback about React 18 Type: Bug

Comments

@zh-lx
Copy link
Contributor

zh-lx commented Apr 12, 2022

React version: 18.0.0

Steps To Reproduce

  1. Write a react component with the following code:
import React, { useState, startTransition } from 'react';

export default function App() {
  const arr = Array(9999).fill(1);
  const [value, setValue] = useState(0);
  const handleInputChange = (e) => {
    console.log(e.target.value);
    startTransition(() => {
      setValue(e.target.value);
    });
  };
  const getValues = () => {
    return arr.map((item, index) => {
      return (
        <div key={index}>
          {value}-{index}
        </div>
      );
    });
  };
  return (
    <div>
      <input type="text" onChange={handleInputChange} />
      <div>{getValues()}</div>
    </div>
  );
}
  1. input content in the <input /> at a very fast speed.
  2. As shown in the figure below, all the contents I entered for the first time have been rendered. But the second time I input 216948261894, only 216948 is rendered, and the following characters are not rendered.
    img

Link to code example:

https://codesandbox.io/s/react18-starttransition-5u1en2?file=/src/App.js

The current behavior

As shown in the figure below, all the contents I input for the first time have been rendered. But the second time I input 216948261894, only 216948 is rendered, and the following characters are not rendered.

The expected behavior

Everything I input should be rendered.

@zh-lx zh-lx added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Apr 12, 2022
@gaearon gaearon added Type: Bug React 18 Bug reports, questions, and general feedback about React 18 and removed Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug labels Apr 12, 2022
@gaearon
Copy link
Collaborator

gaearon commented Apr 12, 2022

I can reproduce this if I type very very very fast. It's worth noting that the input is uncontrolled, so there's nothing wrong with the startTransition call here. It's just that we seem to drop some state updates.

@gaearon gaearon changed the title Bug: some updates haven't been rendered in concurrent mode Bug: Some transition updates haven't been rendered Apr 12, 2022
@zh-lx
Copy link
Contributor Author

zh-lx commented Apr 12, 2022

I can reproduce this if I type very very very fast. It's worth noting that the input is uncontrolled, so there's nothing wrong with the startTransition call here. It's just that we seem to drop some state updates.

I have debugged the react source code for a long time. What I can confirm is that all onChange events and setState are executed normally, and the corresponding update is created for each input.

@gaearon
Copy link
Collaborator

gaearon commented Apr 12, 2022

I think we want to see if that’s the case of an update getting “dropped” from the update queue somehow, or if it’s the case of “forgetting” to commit it.

@zh-lx
Copy link
Contributor Author

zh-lx commented Apr 12, 2022

All updates have executed enqueueUpdate api, I guess that’s the case of an update getting “dropped” from the update queue somehow. By printing the log in enqueueUpdate, I found that after the interleavedUpdate was fully executed, the pending update was not rendered.

The code of my print log is as follows:

 function enqueueUpdate<S, A>(
  fiber: Fiber,
  queue: UpdateQueue<S, A>,
  update: Update<S, A>,
  lane: Lane,
) {
  if (isInterleavedUpdate(fiber, lane)) {
+   console.log('isInterleavedUpdate')
    const interleaved = queue.interleaved;
    if (interleaved === null) {
      // This is the first update. Create a circular list.
      update.next = update;
      // At the end of the current render, this queue's interleaved updates will
      // be transferred to the pending queue.
      pushInterleavedQueue(queue);
    } else {
      update.next = interleaved.next;
      interleaved.next = update;
    }
    queue.interleaved = update;
  } else {
+   console.log('pending')
    const pending = queue.pending;
    if (pending === null) {
      // This is the first update. Create a circular list.
      update.next = update;
    } else {
      update.next = pending.next;
      pending.next = update;
    }
    queue.pending = update;
  }
}

The printed log is shown in the figure below:

image

I hope this is helpful in troubleshooting problems.

@acdlite
Copy link
Collaborator

acdlite commented Apr 12, 2022

My hunch is that it's a bug with the "interleaved updates" check causing the updates to be enqueued in the wrong order

@acdlite
Copy link
Collaborator

acdlite commented Apr 12, 2022

I suspect if you add a enqueueInterleavedUpdates call at the top of the else block it would fix it. Do you mind trying that and see if you're still able to reproduce?

 function enqueueUpdate<S, A>(
  fiber: Fiber,
  queue: UpdateQueue<S, A>,
  update: Update<S, A>,
  lane: Lane,
) {
  if (isInterleavedUpdate(fiber, lane)) {
    const interleaved = queue.interleaved;
    if (interleaved === null) {
      // This is the first update. Create a circular list.
      update.next = update;
      // At the end of the current render, this queue's interleaved updates will
      // be transferred to the pending queue.
      pushInterleavedQueue(queue);
    } else {
      update.next = interleaved.next;
      interleaved.next = update;
    }
    queue.interleaved = update;
  } else {
+   enqueueInterleavedUpdates();
    const pending = queue.pending;
    if (pending === null) {
      // This is the first update. Create a circular list.
      update.next = update;
    } else {
      update.next = pending.next;
      pending.next = update;
    }
    queue.pending = update;
  }
}

@acdlite
Copy link
Collaborator

acdlite commented Apr 12, 2022

^ that's not an appropriate fix, btw, because it has other consequences, but if it works that tells us what the issue is

acdlite added a commit to acdlite/react that referenced this issue Apr 12, 2022
acdlite added a commit to acdlite/react that referenced this issue Apr 12, 2022
@acdlite
Copy link
Collaborator

acdlite commented Apr 12, 2022

I believe this will fix it. Going to a meeting but will check after: #24353

@acdlite
Copy link
Collaborator

acdlite commented Apr 12, 2022

Confirmed that #24353 fixes this: https://codesandbox.io/s/react18-starttransition-forked-dzyyzb?file=/package.json

Thanks so much for the bug report!

@acdlite acdlite closed this as completed Apr 12, 2022
acdlite added a commit that referenced this issue Apr 12, 2022
…dates (#24353)

* Regression test: Interleaved update race condition

Demonstrates the bug reported in #24350.

* Bugfix: Last update wins, even if interleaved

"Interleaved" updates are updates that are scheduled while a render is
already in progress. We put these on a special queue so that they don't
get processed during the current render. Then we transfer them to
the "real" queue after the render has finished.

There was a race condition where an update is received after the render
has finished but before the interleaved update queue had been
transferred, causing the updates to be queued in the wrong order.

The fix I chose is to check if the interleaved updates queue is empty
before adding any update to the real queue. If it's not empty, then
the new update must also be treated as interleaved.
rickhanlonii pushed a commit that referenced this issue Apr 13, 2022
…dates (#24353)

* Regression test: Interleaved update race condition

Demonstrates the bug reported in #24350.

* Bugfix: Last update wins, even if interleaved

"Interleaved" updates are updates that are scheduled while a render is
already in progress. We put these on a special queue so that they don't
get processed during the current render. Then we transfer them to
the "real" queue after the render has finished.

There was a race condition where an update is received after the render
has finished but before the interleaved update queue had been
transferred, causing the updates to be queued in the wrong order.

The fix I chose is to check if the interleaved updates queue is empty
before adding any update to the real queue. If it's not empty, then
the new update must also be treated as interleaved.
rickhanlonii pushed a commit that referenced this issue Apr 14, 2022
…dates (#24353)

* Regression test: Interleaved update race condition

Demonstrates the bug reported in #24350.

* Bugfix: Last update wins, even if interleaved

"Interleaved" updates are updates that are scheduled while a render is
already in progress. We put these on a special queue so that they don't
get processed during the current render. Then we transfer them to
the "real" queue after the render has finished.

There was a race condition where an update is received after the render
has finished but before the interleaved update queue had been
transferred, causing the updates to be queued in the wrong order.

The fix I chose is to check if the interleaved updates queue is empty
before adding any update to the real queue. If it's not empty, then
the new update must also be treated as interleaved.
zhengjitf pushed a commit to zhengjitf/react that referenced this issue Apr 15, 2022
…dates (facebook#24353)

* Regression test: Interleaved update race condition

Demonstrates the bug reported in facebook#24350.

* Bugfix: Last update wins, even if interleaved

"Interleaved" updates are updates that are scheduled while a render is
already in progress. We put these on a special queue so that they don't
get processed during the current render. Then we transfer them to
the "real" queue after the render has finished.

There was a race condition where an update is received after the render
has finished but before the interleaved update queue had been
transferred, causing the updates to be queued in the wrong order.

The fix I chose is to check if the interleaved updates queue is empty
before adding any update to the real queue. If it's not empty, then
the new update must also be treated as interleaved.
rickhanlonii pushed a commit that referenced this issue Apr 19, 2022
…dates (#24353)

* Regression test: Interleaved update race condition

Demonstrates the bug reported in #24350.

* Bugfix: Last update wins, even if interleaved

"Interleaved" updates are updates that are scheduled while a render is
already in progress. We put these on a special queue so that they don't
get processed during the current render. Then we transfer them to
the "real" queue after the render has finished.

There was a race condition where an update is received after the render
has finished but before the interleaved update queue had been
transferred, causing the updates to be queued in the wrong order.

The fix I chose is to check if the interleaved updates queue is empty
before adding any update to the real queue. If it's not empty, then
the new update must also be treated as interleaved.
@gaearon
Copy link
Collaborator

gaearon commented Apr 27, 2022

Fix out in 18.1.0. Thanks again for a great report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
React 18 Bug reports, questions, and general feedback about React 18 Type: Bug
Projects
None yet
Development

No branches or pull requests

3 participants