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: Updates within microtasks are sometimes not batch #24365

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

Bug: Updates within microtasks are sometimes not batch #24365

zh-lx opened this issue Apr 13, 2022 · 10 comments
Labels
Resolution: Stale Automatically closed due to inactivity Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug

Comments

@zh-lx
Copy link
Contributor

zh-lx commented Apr 13, 2022

React version: 18.0.0

Steps To Reproduce

Normal example

The following example is normal. The value of count is incremented by 1 when click the button.

The update of Code Snippet ACode Snippet B and Code Snippet C will be all batch. Because by the time the code executes to Code Snippet A and Code Snippet C, Code Snippet B has already been added to the microtask queue. The flushSyncCallbacks are executed in scheduleMicrotask callback function, therefore, Code Snippet B is executed first and then flushSyncCallbacks is executed.

import { useEffect, useState, useRef } from "react";
export default function App() {
  const [count, setCount] = useState(0);
  const countRef = useRef(-1);

  const handleClickButton = () => {
    // No.B
    Promise.resolve().then(() => {
      setCount(countRef.current + 1);
    });

    // No.A
    setCount(countRef.current + 1);

    // No.C
    setCount(countRef.current + 1);

    // Code Snippet A、Code Snippet B and Code Snippet C are all batch processed
  };

  useEffect(() => {
    countRef.current = count;
  }, [count]);

  return (
    <div>
      <p>count is: {count}</p>
      {/* count will be incremented by 1 when click the button */} 
      <button onClick={handleClickButton}>增加</button>
    </div>
  );
}

link to code example: https://codesandbox.io/s/batch-update-in-native-xpwz33-forked-c4pii4?file=/src/App.js:0-581

Abnormal examples

The following example is abnormal. The value of count is incremented by 2 when click the button.

The update of Code Snippet A and Code Snippet C will be batch, but the update ofCode Snippet B won't be batch.
When the code executes to Code Snippet C, Code Snippet B has not been batch, although Code Snippet B has been added to the microtask queue.

I think all updates added to the microtask queue should be batched when the code executes to the last update on the synchronous execution context.

import { useEffect, useState, useRef } from "react";
export default function App() {
  const [count, setCount] = useState(0);
  const countRef = useRef(-1);

  const handleClickButton = () => {
    // Code Snippet A
    setCount(countRef.current + 1);

    
    Promise.resolve().then(() => {
      // Code Snippet B
      setCount(countRef.current + 1);
    });

    // Code Snippet C
    setCount(countRef.current + 1);

    // Code Snippet A and Code Snippet C will be batch, but Code Snippet B won't be batch
  };

  useEffect(() => {
    countRef.current = count;
  }, [count]);

  return (
    <div>
      {/* count will be incremented by 2 when click the button */} 
      <p>count is: {count}</p>
      <button onClick={handleClickButton}>增加</button>
    </div>
  );
}

Link to code example: https://codesandbox.io/s/batch-update-in-native-xpwz33-xpwz33?file=/src/App.js:0-581

The current behavior

When a setState is executed first in the synchronous execution context, all setState in the microtask queue will not be batched, even if the synchronous execution context still has setState after these microtasks.

The expected behavior

All updates added to the microtask queue should be batched when the code executes to the last update on the synchronous execution stack.

@zh-lx zh-lx added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Apr 13, 2022
@zh-lx zh-lx changed the title Bug: update in a microTask inside native events sometimes won't be batch Bug: Updates within microtasks in native events are sometimes batch Apr 14, 2022
@zh-lx zh-lx changed the title Bug: Updates within microtasks in native events are sometimes batch Bug: Updates within microtasks in native events are sometimes not batch Apr 14, 2022
@zh-lx zh-lx changed the title Bug: Updates within microtasks in native events are sometimes not batch Bug: Updates within microtasks are sometimes not batch Apr 14, 2022
@kh-viewar
Copy link

Also having the same issue with setState and useReducer's dispatch. We use a lot of async functions so definitely a reason to downgrade back to react v17.

@gaearon

This comment was marked as off-topic.

@gaearon
Copy link
Collaborator

gaearon commented Apr 28, 2022

@kh-viewar

Also having the same issue with setState and useReducer's dispatch. We use a lot of async functions so definitely a reason to downgrade back to react v17.

In order to be able to help you, we need to know more. Can you post an example of how the current behavior actually breaks your code? It's not obvious how what you're describing is related to this issue, or even what you are describing exactly. A small sandbox that shows what exactly the breakage is would be very helpful.

In this issue, @zh-lx documented an inconsistency in when batching is applied or not applied. While we'd like to fix it, it's not clear to me why this inconsistency would actually cause a problem for your code.

Comments like "definitely a reason to downgrade back" are especially not helpful since if you don't provide more details, we can't solve the issue that prevents you from upgrading.

@frontend-winter
Copy link

I don't think so. The existing model is logical.

@kh-viewar
Copy link

The main problem is that I'm dispatching actions with useReducer (inside an async callback) and the reducer code is never executed. Will try to create a basic sample.

@kh-viewar
Copy link

Ok found out the issue, maybe it's not related to the initial bug described in this ticket. We run the react app inside a native app's WebView on mobile devices. To communicate between the native app and JavaScript Coherent UI is used. For this a new iframe is created and destroyed again. Somehow the batched state changes are not flushed if the promise inside changes the DOM (= attaching an iframe).

Sample code:

async function sendIframe() {
  return new Promise(resolve => {
      const document = window.document;
      const frame = document.createElement('iframe');

      document.documentElement.appendChild(frame);
      document.documentElement.removeChild(frame);

      setTimeout(() => resolve(true), 2000);
    }
  );
}

const App = () => {
  const [showSpinner, setShowSpinner] = useState(false);
  
  const process = React.useCallback(async() => {
    setShowSpinner(true);
    console.log('start');

    // This method roughly resembles a Coherent UI communication.
    // Both state changes (setShowSpinner) are never executed.
    await sendIframe();

    setShowSpinner(false);
    console.log('done');
  }, []);

  const renderSpinner = useCallback(() => {
    return (
      <div>Spinner</div>
    );
  }, []);
  
  return (
    <>
      {showSpinner && renderSpinner()}
      <button onClick={() => process()}>Upload</button>
    </>
  ); 
};

const rootElement = document.getElementById('app');
const root = createRoot(rootElement);
root.render(<App />);

@gaearon
Copy link
Collaborator

gaearon commented Apr 28, 2022

Would you mind filing a separate issue for this with a runnable example?

@zh-lx
Copy link
Contributor Author

zh-lx commented Apr 28, 2022

Would you mind filing a separate issue for this with a runnable example?

I'd love to. You can compare the two working examples below.

In this case, clicking the button once increases the value of count by one: Promise isn't batched example

In this case, clicking the button once increases the value of count by two: Promise is batched example

Copy link

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

@github-actions github-actions bot added the Resolution: Stale Automatically closed due to inactivity label Apr 10, 2024
Copy link

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Stale Automatically closed due to inactivity Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants