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

useEffect can very quickly consume free quotas or cost money when used with 3rd party services #15188

Closed
PutziSan opened this issue Mar 22, 2019 · 8 comments

Comments

@PutziSan
Copy link

Do you want to request a feature or report a bug?
Feature / Documentation-Request

What is the current behavior?
When I developed my app last week with useEffect and firebase firestore, it happened to me that my effect used up the 20k-write limit within about 20 seconds. Of course this was a bug introduced by myself, but if I had been in a pay-as-you-go plan it could have cost me some money. I now use a custom hook as useEffect, which counts in development whether a hook is executed too often in 500ms and if so, it throws an error.

What is the expected behavior?
I'm not sure how you could solve this on your side. Of course you could do the check in development-mode, but that would probably trigger existing projects too much. However, a small hint in the documentation would be good that you should take care during development that useEffect can quickly lead to this behavior and that you should be careful when using other 3rd-party services that have a quota or have to be paid.

I just wanted to share my experiences while developing a "real" app. If you can't or won't do anything here, you are welcome to close the issue.

@gaearon
Copy link
Collaborator

gaearon commented Mar 22, 2019

Yeah, this has been bugging me too. To be clear it’s not entirely a new problem. The same could happen if you unconditionally fetched in componentDidUpdate and then added an animation to a parent component. But I agree it comes up more with useEffect.

Can you share the exact code snippet you used? It’s kinda tricky to catch early. I am adding mitigations for some cases in #15184 and #15180 but these are mostly for sync loops. Catching async ones is way harder.

@PutziSan
Copy link
Author

Can you share the exact code snippet you used?

Do you mean the snippet which caused the bug? This is the condensed variant:

function Comp(props) {
  const [data, setData] = useState();

  useEffect(async () => {
    if (!data) {
      return;
    }

    const newData = {
      timestamp: Date.now(),
      content: data
    };

    await firebase
      .firestore()
      .collection("test")
      .doc(props.id)
      .update(newData);
  }, [data]);

  useEffect(() => {
    const unsubscribe = firebase
      .firestore()
      .collection("test")
      .doc(props.id)
      // the bug:
      // onSnapshot changes data, whereupon the other effect is activated,
      // which again triggered onSnapshot, .....
      // and onSnapshot is also called synchronously:
      // "Local writes in your app will invoke snapshot listeners immediately."
      // https://firebase.google.com/docs/firestore/query-data/listen
      .onSnapshot(snapshot => {
        setData(snapshot.data().content);
      });

    return unsubscribe;
  }, [props.id]);

  return (
    <div>
      <input onChange={e => setData({ msg: e.target.value })} />
    </div>
  );
}

@gaearon
Copy link
Collaborator

gaearon commented Mar 22, 2019

I'm not familiar with Firebase — could you explain more about why there are two effects, and what was the intended logic? Thank you.

@PutziSan
Copy link
Author

PutziSan commented Mar 22, 2019

The principle is something like google docs: We have one document and 2 users, both users can write in the document at the same time and see each other's changes "in real time".

And the 2 effects are correspondingly there for the 2 actions: write into the document at own changes and read from the document when the other user writes into the document.

The problem was that the first time I implemented it, I immediately got my own changes mirrored back, which then leads to an infinity loop.

@gaearon
Copy link
Collaborator

gaearon commented Mar 23, 2019

Do you think it could be something that Firebase bindings could handle? There are many other cases where you could accidentally call something in a loop. It seems like Firebase library itself could warn or error because it’s obvious you don’t want to fire off hundreds of requests in a second. In other words it would make sense to me if the error happened close to where we actually know the behavior isn’t desirable — which is right next to the network layer.

@PutziSan
Copy link
Author

PutziSan commented Mar 23, 2019

I see what you mean. For me the solution is to use a custoom hook as useEffect. This is faster and easier to implement for me and then also applies to all other cases. I have to opt-in to allow an effect to really be called more often than it should be "normal". I have defined "normal" as an estimate for myself: 5 calls within 500 ms. This seems to work quite well so far, it fails if it wasn't correct and otherwise it goes through.

Here is the code which I use to "observe" my effects (or you can take a look at this CodeSandbox-demo):

function useObservedEffect(
  fn,
  deps,
  { maxCalls = 5, maxCallIntervalMs = 500 } = {}
) {
  /* eslint-disable react-hooks/exhaustive-deps */
  // effects inside if is okay here, since the result of
  // `process.env.NODE_ENV === "development"` is static per build
  if (process.env.NODE_ENV === "production") {
    useEffect(fn, deps);
  } else {
    // in development-mode, check if the effect will be called too often
    const callCountRef = useRef(0);
    const [error, setError] = useState();

    if (error) {
      throw error;
    }

    // Reset the counter every X milliseconds so we count the calls only per interval
    useInterval(
      () => {
        callCountRef.current = 0;
      },
      // if maxCallIntervalMs === 0, the verification should not be performed.
      // In these cases, set the interval to the maximum possible value
      // so that it is not executed senselessly often.
      maxCallIntervalMs > 0 ? maxCallIntervalMs : Number.MAX_VALUE
    );

    useEffect(() => {
      // Increase the counter corresponding to the calls in the last interval.
      callCountRef.current++;

      // if maxCallIntervalMs === 0, the verification should not be performed.
      if (maxCallIntervalMs > 0) {
        if (callCountRef.current > maxCalls) {
          // if the effect is called too often, set an error and do not execute the provided effect-function
          // the error will throw in next render
          setError(new Error(`... (error-message)`));
          return;
        }
      }

      fn();
    }, deps);
  }
}

I cannot see any disadvantages in this approach for me, but I could imagine that it could not fit so well into the react-package directly, because effect functions suddenly react differently in development than in production. Some users might not find this good

@elzup
Copy link

elzup commented Jul 17, 2021

I love react, but I'm melting Firebase quota all the time in this pit.

@PutziSan
Copy link
Author

I would recommend to use https://github.com/kentcdodds/stop-runaway-react-effects nowadays. It is basically my proposed solution from above, but overwrites the useEffect hook directly during development.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants