Schedule uncommitted reaction cleanup

[RoystonS]

(Recreated from PR #120 as I screwed up some repo stuff.)

This PR builds on PR #119 and introduces some fixes for #53:

• a unit test for checking that reactions are properly disposed for components that were double-rendered before being committed (e.g. by ConcurrentMode or StrictMode).
• a fix which schedules the cleanup of such reactions.

Note that there's one aspect I'm unsure of, both of how to test or fix in a nice way, so I'm pushing this up for feedback. cc: @mweststrate, @FredyC

The happy day case, which I've tested for, and coded for:

1. render phase 1:
   1. Component instance A gets rendered, creating reaction R1, which we track
   2. we trigger a timer to check for leaked reactions
   3. rendering is aborted and restarts, or StrictMode triggers a second render in order to root out trouble
   4. Component instance A gets rendered, creating reaction R2, which we track
2. commit phase 1:
   1. Component instance A commits, and we remove R2 from the 'leaked reactions' list
3. render phase 2:
   1. Component instance B gets rendered, creating reaction R3, which we track
   2. second render triggered
   3. Component instance B gets rendered, creating reaction R4, which we track
4. commit phase 2:
   1. Component instance B commits, and we remove R4 from the 'leaked reactions' list
5. cleanup timer ticks:
   1. R1 and R3 are disposed. All is well. Everybody is happy.

But there's an edge case I'm concerned about (which is why I don't think that this PR is sufficient). What if it's been a while since the timer was kicked off, but the latest render phase hasn't had a corresponding commit phase yet?

1. render phase 1:
   1. Component instance A gets rendered, creating reaction R1, which we track
   2. we trigger a timer to check for leaked reactions
   3. rendering is aborted and restarts, or StrictMode triggers a second render in order to root out trouble
   4. Component instance A gets rendered, creating reaction R2, which we track
2. commit phase 1:
   1. Component instance A commits, and we remove R2 from the 'leaked reactions' list
3. render phase 2:
   1. Component instance B gets rendered, creating reaction R3, which we track
   2. second render triggered
   3. Component instance B gets rendered, creating reaction R4, which we track
4. cleanup timer ticks (between render and commit - this can happen, right?) :
   1. R1, R3 and R4 are disposed
5. commit phase 2:
   1. Component instance A commits, and the world ends because R4 has previously been disposed by the cleanup schedule

I think, to do this properly, we need to record that a reaction is safe to clean up only once a commit phase has happened?

[RoystonS]

Whilst, if it could be made to work, this approach would be very efficient, I'm unconvinced that it can be made to work 100% reliably as I'm not sure there's a reliable way to track whether a commit has occurred or not other than to use useEffect or useLayoutEffect component-by-component. The 'render once without observation' and 'subscribe at commit time' may be the only way to make it work correctly. :-(

[coveralls]

Coverage increased (+0.1%) to 99.422% when pulling 6c78e5b on RoystonS:schedule-uncommitted-reaction-cleanup into bf3a6c9 on mobxjs:master.

[danielkcz]

It would great if @gaearon could chime in here as it's fairly heavy stuff from React internals.

If I am understanding edge case correctly, it means there is a risk of throwing away the Reaction that's not stale actually? Yea, that would be bad, let's not do that :)

Sadly, I have no idea right now how to deal with it. Wish it would be possible to write a test for that edge case.

[RoystonS]

Yes, @FredyC, I think in that edge case it would be possible to throw away a non-leaked Reaction. It might be possible to put potentially-leaked reactions in a bucket somewhere, and then when we see a commit phase run to completion, anything in that bucket is leaked. But I'm not at all sure if there's a reliable public way to track that.

I've thought of a way that might enable the unit testing of such a case: trigger the cleanup timeout from the end of a render phase, causing the cleanup to happen before commit. I'll try to get such a test together.

In the meantime, I've also thrown together a real useState+useEffect-based version of the fix - the one I've been trying to avoid. It doesn't create a reaction until the commit phase, so there's no possibility of leakage, but it does require every observer component to do a full second render cycle so it's a little less efficient. I have a suspicion that @gaearon has said in the past that we shouldn't be afraid of such a cycle in general (back about a year ago where we were being steered towards doing more work in componentDidMount rather than the xxxWillxxx lifecycle methods) but it still makes me a little uneasy.

It passes the StrictMode and leak tests but it blows up many of the other mobx-react-lite tests because many of them aggressively count the number of times components are rendered, and this obviously changes that count significantly.

Here's that code if you want to see it, compared against the current trunk implementation of useObserver:
RoystonS@de800f5

[danielkcz]

I have a suspicion that @gaearon has said in the past that we shouldn't be afraid of such a cycle in general (back about a year ago where we were being steered towards doing more work in componentDidMount rather than the xxxWillxxx lifecycle methods) but it still makes me a little uneasy.

Yea, I am not too excited about it either. What if the observed component is somewhat heavy, re-rendering it twice is fairly awkward and might be a reason why people would avoid MobX in React.

At this point, it feels like a matter of choosing the right tradeoff. Either there would be some lingering Reactions or we would be producing extra rerender. What is worse? I would be voting for keeping lingering Reactions as it would be a problem in really intensive apps only.

What if we include some safety net to the current solution? I mean we have a list of (theoretically) uncommitted Reactions. We could keep time when the reaction last run and dispose of it only if it breaks it some threshold. Sure, it's not an exact science either, but it does lower the risk of disposing of something that's actually used.

[danielkcz]

Btw, regarding your solution with useState. Initially, I thought there would be a risk of missing some updates between the first render and the second one. However, since the second render actually reruns the component, it's safe to say, that any possible update that occurred is covered by that render.

But yea, there is still "the problem" of extra render that might be fairly annoying in some situations.

[danielkcz]

Another idea. What if a timer was more like a debounce? Instead of static interval of cleanup, every call to the scheduleCleanupOfReactionIfNotCommitted would remove the previous timer and start a new one. Effectively it would mean that no cleanup will happen until the app becomes somewhat idle (no rendering happening).

I don't think it's very common to have apps that are mounting/unmounting many observed components very often. And even if they are, it would only mean bigger memory footprint that would be cleared later.

Edit: Tried the implementation 👇 Tests are passing. Do you think it covers the mentioned edge case?

[RoystonS]

That's not a bad idea. I can't think of a scenario where it would fail, but the global debounce does make me a little nervous: an application doing real-time animations might not have a long enough idle period.

The other way to do it is to have a per-reaction debounce instead of a global debounce: track the age of a 'potentially leaked' reaction, and if it's older than a certain amount, it's eligible for cleanup. No global idleness needed.

[danielkcz]

Yea I agree, was thinking the same. Besides global objects are always a pain when writing app tests. It could cause some flakiness.

I am not sure I follow your idea though. Where would you keep that time? Do you mean writing it directly on reaction object? That's nasty :) But we can utilize WeakMap I suppose. And where would be a cleanup timer running? I don't know, I don't seem to be able to imagine how that would work.

[RoystonS]

I'll code it up. Much clearer than English. :-)

But basically, imagine, instead of pushing a reaction into the 'cleanup' list, pushing a { reaction, at: Date.now() } object, and in the cleanup, we only clean up things with an at older than, say 5000ms. The cleanup timer keeps running every, say 1000ms, whilst there are things in the cleanup list.

[RoystonS]

(Btw, if we could use Set or Map it could be made more efficient. But we can't assume those... or can we?)

[danielkcz]

What I suppose would be super safe and not global at all is to have a separate timer for each reaction. It starts when the Reaction it's created and if it's not committed within like 50ms (probably less) it's safe to assume it's lingering. It's most likely overkill, but safe one! :)

I am thinking if there isn't a potential for memory leak within application tests. With a global timer if you use eg. jest.useFakeTimers and don't move time forward enough, it won't dispose of those reactions. It's probably ok for CI tests, but in development and watch mode, it could possibly pile up over time. Not sure if memory heap is reused over test runs though.

(Btw, if we could use Set or Map it could be made more efficient. But we can't assume those... or can we?)

Well, according to the compat table, the WeakMap basic functionality is present even in the IE11 and it should be enough for our use case.

[RoystonS]

Tracking the creation time of each reaction is equivalent to having a separate timer for each reaction.

In tests, if you don't push the fake timers ahead, you're leaking reactions to what's probably a temporary set of data that will be recreated in the next tests, so you probably haven't leaked anything at all?

[danielkcz]

Tracking the creation time of each reaction is equivalent to having a separate timer for each reaction.

Huh. So you are saying it's not more expensive to have numerous setTimeout? I can imagine there is only a single timer running in a browser and just comparing times, but is that true for every environment? What about some multi-threading? I heard Next.JS has some sort of that. If it's ok, then let's go ahead and make it ultra safe :)

Btw, we should also check for isUsingStaticRendering and don't worry about any cleanup in that case. Although I suppose that no schedule will be called anyway, so I guess it's safe.

[RoystonS]

I think it would be more expensive to have a separate real timer per reaction, so I'm suggesting just having one real timer running, checking simple numeric timeouts on each 'potentially leaked reaction'.

The other thing worth considering in unit tests is that all this leakage is only an issue in strict or concurrent mode. (We do now run all our unit tests in strict mode, which has been an exciting journey.) But right now, mobx-react-lite always leaks reactions in that case, so whatever we do here is a win. :)

[danielkcz]

Alright, let's do a global timer and WeakMap then :)

[RoystonS]

I'm writing some tests first. Red, Green, Refactor and all that.

(I'm also suspicious about what happens if an observable change occurs between first render and first commit: the change in #119 would mean that gets ignored, and possibly never handled, so we may need to track that a MobX change occurred before commit, which would need to trigger a second render. I'll get a test together for it.)

[danielkcz]

@RoystonS You are right there is that risk for sure. That's kinda why I wanted to useState originally so it does re-render after commit and a potential update would be handled by that.

Perhaps we shouldn't be overthinking this. Maybe it's not so big performance problem to have that double re-render there. There is a chance that React can optimize it and only one render happens in the end. Also, the silver lining is that wouldn't even need this custom garbage collector doing it that way.

Btw, just realized, if we do NOT use render function from RTL we can effectively test commit phases because without calling an act (which RTL does), no useEffect would be executed. I am not sure how exactly you want to test it, but this is an easy way imo :)

[RoystonS]

Right. That was a bit of a mammoth run. Two new unit tests.

The first confirms and demonstrates the suspicion I had after PR #119, that changes made to an observable will be lost if they occur between its render and commit. This unit test fails both before and after PR #119, so that PR didn't make it worse.

The second (which is a rather harder test to write) demonstrates that if the scheduled cleanup task comes in between a component having its first render and its first useEffect call, then reactions created in that render (which haven't leaked: they just haven't been 'confirmed' in useEffect yet) will be incorrectly tidied up. It's not a perfect unit test by any means: it would be possible to make this test pass (but not fixing the real issue) by moving the detection from useEffect to useLayoutEffect but that's only because I'm not able to use full Concurrent mode here, and so we're not properly asynchronous. Once Concurrent mode goes official, this test could be changed over.

Armed with failing unit tests we can now look at solutions.

Fixing the first is easy: we need to track that there was a change before commit, and trigger a re-render during commit.

Fixing the second requires some way to track that a reaction has actually been leaked from a discarded render. It's probably sufficient to consider that a reaction is leaked if it was created 'some time ago' (e.g. > 10 seconds?) I need to do a bit more research into Suspense and some of the way fiber render trees are paused. Hopefully facebook/react#15317 might help clarify.

[RoystonS]

Here's another unit test that illustrates a further issue with the options. I already had a test for checking that observable changes that occur between first render and commit shouldn't be lost, but I was only running that test in Strict Mode. A check against our codebase (with 100+ observer components) flagged up a case where it was an issue. So I've added a non-strict mode test. I'll continue the discussion in PR #119 as we have a problem, @FredyC...

[danielkcz]

Honestly, I got a bit lost in the solution, so I am just going to trust both of you it's alright :)

@RoystonS Still planning on catching up with tests? Or is that 7% drop something you are unable to test reliably?

[RoystonS]

Honestly, I got a bit lost in the solution, so I am just going to trust both of you it's alright :)

@RoystonS Still planning on catching up with tests? Or is that 7% drop something you are unable to test reliably?

I'm confused. The code that is being flagged as uncovered (i.e. the cleanup stuff) is tested. I think something funny might have happened with the tests when I renamed the test file. I thought it was all covered. Will check.

[RoystonS]

That's better. It was indeed an issue of the test file name: when I renamed it, I forgot the .test. suffix so my new tests weren't running. Now they are, there's actually an increase in coverage. :-)

[danielkcz]

Nice! :) So I take it we are ready to merge it? It's battle tested I assume? :)

[RoystonS]

I'm just extracting the tracking and cleanup logic out of useObserver as @mweststrate suggested, and then I think it's probably ready. I'll get that done within the next few minutes.

Define "battle-tested". I've certainly been running it in our codebase for the last 2 weeks since my previous comments and we've not seen any issues but it's been Easter and we've not done detailed memory heap analysis, etc.

Regarding readiness for merge... it's "quite a non-trivial change": as I mentioned before, I did have one unit test that needed fixing up, so I don't know whether you'd want to consider a major version bump in order to signal potential breaking changes? If you don't want a major version bump, it might be worth considering pushing up via a non-default dist-tag (e.g. '@next') so that people can elect into it to try it before making it mainstream?

[danielkcz]

Yes, the @next tag sounds like the best option right now. I will take care of it by tomorrow. Many thanks for hard work here.

[danielkcz]

Alright, published under @next tag :) Thanks again.

[Bnaya]

As s followup for this PR:
Please join the discussion at
mobxjs/mobx#2562
Regarding using FinalizationRegistry to dispose reactions on supported platforms