HY-4957 Alert devs about batchedRedraw race condition, and log if it happens in prod. ⚠️

[tomconnell-wf]

Ultimate problem:

If a store is disposing or disposed before batch redraws, the next componentWillMount will try to subscribe to the disposed store stream, resulting in exceptions.

How it was fixed:

Give useful information in dev, and send a log so we know if it is happening much in prod.

Testing suggestions:

CI passes

Potential areas of regression:

Triggering

---

FYA: @greglittlefield-wf @aaronlademann-wf @jacehensley-wf @clairesarsam-wf @joelleibow-wf @georgelesica-wf

[aviary-wf]

Raven

Number of Findings: 0

[tomconnell-wf]

I'm open to better ways to check the subscriptions in unit tests.

[codecov-io]

Codecov Report

Merging #72 into master will decrease coverage by 0.07%.
The diff coverage is 66.67%.

@@            Coverage Diff             @@
##           master      #72      +/-   ##
==========================================
- Coverage    97.7%   97.63%   -0.06%     
==========================================
  Files          28       28              
  Lines        1389     1392       +3     
==========================================
+ Hits         1357     1359       +2     
- Misses         32       33       +1

[stephenbush-wf]

Alternatively (or supplementarily), this could test that rendering does not cause an exception (when the store is listened to while disposing). Technically, that tests the failure case that we're addressing, but it also asserts the expected behavior in a weird way that is dependent on behavior from another repo. But it also wouldn't require the subscriptions list to be exposed.

🤷‍♂️

[greglittlefield-wf]

Good point; that could be done like so:

test('does not subscribe to a store that is disposed or disposing', () async {
  await store.dispose();
  expect(() {
    render(TestDefault()..store = store);
  }, returnsNormally);
});

[greglittlefield-wf]

@tomconnell-wf Thanks for the PR!

Question, though; if the store is disposed or disposing, can the newly-mounted component even recover? Or is this a case where the component will, in most cases, be unmounted by whatever's disposing the store immediately after mounting?

I'm also a little fuzzy on BatchedRedraws, so I'm going to familiarize myself a bit more so I can understand the problem.

[tomconnell-wf]

My unit test was disposing a store, before the next animation frame, where the BatchedRedraws were happening. As a result, that listen call that is guarded would fail on a close stream.

I could have awaited the next animation frame, in that test, but this seemed like something that consumers shouldn't have to worry about. I wanted to fix it somewhere that didn't take others hours to track down. The batched redraw mixin doesn't know anything about the store, so the fluxComponent seemed like the correct place to change.

That being said, I'm a little fuzzy on how disposing ties in to the react lifecycle. I'm unclear on what should happen, because as it was, I think the components never would have rendered, because of the uncaught exception.

[greglittlefield-wf]

It's seeming to me like in this case, the component should throw an error if mounted with a disposing/disposed store, as opposed to failing to register silently—in a production app, that would definitely be a problem we'd want to know about.

I could have awaited the next animation frame, in that test, but this seemed like something that consumers shouldn't have to worry about. I wanted to fix it somewhere that didn't take others hours to track down.

What about something like this, instead?

 handlers.forEach((store, handler) {
+  assert(!store.isDisposedOrDisposing,
+        'Cannot listen to a disposed/disposing Store. '
+        'This can be caused by BatchedRedraws mounting the component asynchronously after the store has been disposed. '
+        'If you're in a test environment, try adding an `await window.animationFrame;` before disposing your store.');
+
   StreamSubscription subscription = store.listen(handler);
   _subscriptions.add(subscription);
 });

But there's still the possibility that this race condition could happen in a production environment... I'm having a hard time thinking of how we could effectively guard against this... @evanweible-wf @trentgrover-wf, ideas?

[tomconnell-wf]

I would be happy to compromise with an assert, although I think I would prefer a log, so we could track if it is happening in prod. Maybe a hybrid of both for better dev ergonomics.

[tomconnell-wf]

@greglittlefield-wf How does this seem?

[greglittlefield-wf]

Could you please add logging to pubspec.yaml?

[greglittlefield-wf]

+1 other than my one comment

[evanweible-wf]

space after if

[greglittlefield-wf]

I wasn't going to say anything since we don't use dartfmt :P

[evanweible-wf]

😈

[greglittlefield-wf]

+1

[patkujawa-wf]

There's not clear guidance that I can find, but I think we should clearListeners on the Logger when things are disposed.

[greglittlefield-wf]

Mm, good idea; we don't want this potentially leaking for every FluxComponent instance.

@tomconnell-wf What about making this instance static / top-level instead?

[greglittlefield-wf]

Or cleaning it up in componentWillUnmount (shrug)

[tomconnell-wf]

Sure

[patkujawa-wf]

+1 CR, FWIW

[greglittlefield-wf]

+1

[patkujawa-wf]

Ah, good idea. +1 CR again as well.

[patkujawa-wf]

+10 CI passed :)

[tomconnell-wf]

@greglittlefield-wf is this enough for merge in this repo?

[greglittlefield-wf]

@tomconnell-wf We just need a QA +1 now, and it can be merged

[greglittlefield-wf]

I'l do that since we have a +10 :)

[greglittlefield-wf]

QA +1

• Dev +1's
• Dev/QA +10
• All unit tests pass
• Rosie ran/Rosie comment displays expected info
• Dependency Scan Clean

Merging.