The BehaviorSubject of useObservable could cause resource leaks

[darkyzhou]

Root cause:

export function useObservableInternal(...): Observable<TOutput> {
  if (!inputs) {
    return useState(init as () => Observable<TOutput>)[0]
  }

  const [inputs$] = useState(() => new BehaviorSubject(inputs))
  const [source$] = useState(() => init(inputs$))

  const firstEffectRef = useRef(true)
  useCustomEffect(() => {
    if (firstEffectRef.current) {
      firstEffectRef.current = false
      return
    }
    inputs$.next(inputs)
  }, inputs)

  // No `inputs$.complete()` presents :/

  return source$
}

When using operators like shareReplay(), they typically rely on the source observable(i.e. the BehaviorSubject inside useObservable()) to send a complete signal in order to unsubscribe the internal ReplaySubject from it. If no complete signal is received, resource leaks could occur.

Stackblitz URL: https://stackblitz.com/edit/stackblitz-starters-ghtjf1?devToolsHeight=33&file=src%2FApp.tsx

The example above is using RxJS 6. Haven't tested on RxJS 7 yet.

• In the example, we use a toggle button to control the existence of FantasyGauge.
• In FantasyGauge we use a modified version of useObservable with myShareReplay (which just logs some actions on top of original shareReplay logics from RxJS 6.2.1).
• When FantasyGauge is destroyed, the BehaviorSubject of myUseObservable still holds some observers. See BEFORE.mp4 below.
• When added inputs$.complete(), the BehaviorSubject no longer holds any observers. See AFTER.mp4 below.

I also find that useObservableRef and useObservableCallback both use BehaviorSubject or Subject under the hood, and both of them don't seem to call complete() either. So I suspect these hooks might also be vulnerable to resource leaks.

BEFORE.mp4

AFTER.mp4

[darkyzhou]

Any comments? Would love to open a PR for this.

[crimx]

Hi thanks for the info and sorry for the late reply. I have not come up with a clean and safe solution for calling complete inside components. If you have an idea on how to fix it PR is welcomed.

[crimx]

Two approaches I could think of:

1. Going fully React idiomatic. The observable created by useObservable would change, so that we can use useEffect to complete the old BehaviorSubject and replace with a new one. But this approach requires listing deps explicitly like other hooks useXX(() => {}, [...deps]) which is cumbersome and a breaking change.
2. Delay complete with setTimeout in useEffect so that extra effects triggered in StrictMode are cancelled. This is hacky but should work as expected.

[darkyzhou]

Thank you for the replies.

As for the first approach, I think it introduces too many breaking changes and breaks the simplicity of the library.

As for the second one, I don't get what you mean. By "extra effects", do you mean the fact that in StrctMode the cleanup function of useEffect will be called twice? I think at least we could introduce another ref like firstEffectRef to work around that.

[crimx]

Not just the cleanup but the useEffect will run twice. https://react.dev/reference/react/StrictMode#fixing-bugs-found-by-re-running-effects-in-development

[darkyzhou]

Sorry for the late reply. I get what you are referring to and notice the difficulties of leveraging the useEffect() to call complete(). While I understand the need to defer to the next macrotask or microtask, it's not always feasible due to the risk of leaking setState() calls(e.g. users of useObservable() might accidentally call setState() after the component is destroyed).

An alternative could be to require users to disable StrictMode, similar to what LeetCode does, as StrictMode does not seem to be a useful utility. I personally find it counterproductive that useEffect() is called twice simply to prevent "user mistakes", especially when it compromises the ability to synchronize with external systems like RxJS. What's more frustrating is the apparent lack of a way to detect whether it's in StrictMode.

With the release of React 18, it seems that useEffect() no longer truly functions as an effect, but rather as a "side-effect-less effect". The expectation that the cleanup function should completely reverse the effects of the hook seems unrealistic. This reminds me of a time when I attempted to use it as an onMount() hook to display a toast to the user upon the creation of a component, only to be surprised by two toasts appearing. Both the document and stackoverflow say I am misusing the hook while not being able to offer me a "right" way to achieve the goal.

The changes to useEffect() in StrictMode, in my opinion, have not been for the better. It feels like the essence of what made it so useful has been lost, and it's become more of a hindrance than a help in certain scenarios.

[crimx]

The reason that React calls useEffect twice in StrictMode is that by design, React wants you to create side effect and clean it up in a single useEffect, so that in concurrent mode, the React scheduler would have the ability to interrupt a rendering and resume at some point later.

I too think this is an unrealistic design and go against ergonomics. They tried to push it for over two years and responses from the community does not seem well (too hard or even impossible for library authors to implement).

Nevertheless, I still want to keep it from breaking in StrictMode. The LeetCode team has moved away from rxjs-hooks for quite a while that is why I created observable-hooks.

I also lean toward the second approach. To avoid delaying complete maybe we can detect the StrictMode ahead and skip the first cleanup directly.

[darkyzhou]

I'm of the opinion that this doesn't relate to the concurrent mode features. Everything that takes place within useEffect() occurs during the commit phase. In concurrent mode, React does indeed interrupt, but this only happens during the render phases. Once we reach the commit phase, the process becomes uninterruptible. The only rationale I can conceive for the React team pushing this forward, despite reservations and skepticism from the community, is that it might stem from a strategic decision made by a high-ranking member of the team.

Detecting StrictMode seems challenging, as <StrictMode> is essentially a Symbol like Fragment, and the real magic unfolds within the reconciler. I'm keen on exploring potential solutions that could align with StrictMode. As for now, I'm considering forking the project and tailoring it exclusively for React 17 and earlier versions since the project I'm working on still uses React 16.

[jorroll]

Just stumbling across this issue. It's not clear to me that there is an actual problem here @darkyzhou, but maybe I'm not fully understanding it.

The issue at hand is that the internal BehaviorSubject created by useObservable is, depending on how it's used, retaining a reference to a shareReply observer created by the initialization factory function provided to useObservable. This observer should be removed when useObservable is unmounted, but that's not happening. Is that correct?

If this is accurate, I don't think this actually matters as, in this scenario, the BehaviorSubject is also unmounted and free to be garbage collected. The observer will still be garbage collected at the same time the BehaviorSubject is. It looks like the two will always be garbage collected when the use of shareReplay is, so it doesn't look like there's an actual problem here.

Am I misunderstanding something @darkyzhou ? Is it possible for the observable created by shareReply to be otherwise freed from memory before the BehaviorSubject is?

[darkyzhou]

Just stumbling across this issue. It's not clear to me that there is an actual problem here @darkyzhou, but maybe I'm not fully understanding it.

The issue at hand is that the internal BehaviorSubject created by useObservable is, depending on how it's used, retaining a reference to a shareReply observer created by the initialization factory function provided to useObservable. This observer should be removed when useObservable is unmounted, but that's not happening. Is that correct?

If this is accurate, I don't think this actually matters as, in this scenario, the BehaviorSubject is also unmounted and free to be garbage collected. The observer will still be garbage collected at the same time the BehaviorSubject is. It looks like the two will always be garbage collected when the use of shareReplay is, so it doesn't look like there's an actual problem here.

Am I misunderstanding something @darkyzhou ? Is it possible for the observable created by shareReply to be otherwise freed from memory before the BehaviorSubject is?

You are right. As for garbage collection there seems to be no problems. Sorry for not being clear about what I am actually referring to for "resource leaks".

When I mention "resources", I'm referring to ongoing side effects that need to be cancelled or explicitly signaled to stop when the Observables they are replying on is complete()-ed. When these Observables, created by the hook, are consumed locally within a component, it appears to be fine as the subscriptions are cancelled when the component is destroyed. However, potential issues could arise when these Observables are referenced across multiple components or modules that do not share the same lifecycle.

[jorroll]

However, potential issues could arise when these Observables are referenced across multiple components or modules that do not share the same lifecycle.

Is this possible though? The only failure scenario that I can see occurs when shareReplay is used inside the factory function of useObservable(). And in this case, the observable created by the factory function will always be local to that component (and never reused elsewhere). If you pass in an already made observable (i.e. not passing a factory function), then you are also choosing not to use the inputs$ feature of the useObservable hook and thus you are also not using the internal BehaviorSubject.

If you try a hybrid approach, e.g. using a factory function that, internally, makes use of an observable created in outside context, then presumably that outside observable would be used inside a switchMap or similar operator by the factory function, and this operator will ensure that the outside observable's subscription is completed on unsubscribe.

For example:

declare const otherObservable: Observable<boolean>;

useObservable(
  (inputs$) => 
    inputs$.pipe(
      switchMap(([inputValue]) =>
        return inputValue ? otherObservable : NEVER
      )
    )
)

I guess I don't know about mergeMap though. I very rarely use mergeMap so I'm not sure if it ever sends completed events 🤔.

[Johnius]

Would something like this work:

const useReactLifecycleObservable = (): Observable<unknown> => {

    const reactLifecycleEndSubject = useMemo(() => new Subject<unknown>(), []);

    useEffect(() => () => {
        reactLifecycleEndSubject.next();
        reactLifecycleEndSubject.complete();
    }, [reactLifecycleEndSubject]);

    return reactLifecycleEndSubject;
}

const useLifecycleAwareObservable = <
    TOutput,
    TInputs extends Readonly<any[]>,
    TObservable extends Observable<TOutput>,
>(
    init: (inputs$: Observable<[...TInputs]>) => TObservable,
    inputs: [...TInputs],
): TObservable => {

    const lifecycleEnd$ = useReactLifecycleObservable();

    return useObservable<TOutput, TInputs, TObservable>(
        (inputs$) => {
            return init(inputs$.pipe(takeUntil(lifecycleEnd$)));
        },
        inputs
    );
};