StoreConnector.shouldUpdateModel and VmFactory.fromStore called with different state instances

[kuhnroyal]

It is possible for StoreConnector.shouldUpdateModel to be called with a different state instance than VmFactory.fromStore. So after verifying the correct state for the viewmodel, the state can change into an invalid state and fromStore is called with the invalid state.

[marcglasberg]

I will give this priority, but could you please provide some reproducible code? Maybe even better, a failing test?

[kuhnroyal]

Not sure, I will try. The code I have is not easy to reproduce. I think it is somehow related to the use of NavigateAction.

[kuhnroyal]

Looking at the source I think I found the potential problem site inside _StoreStreamListenerState:

    if (widget.shouldUpdateModel != null) {
      _stream = _stream.where((state) => widget.shouldUpdateModel(state));
    }

    stream = _stream.map((_) => getLatestValue());

The state that is used to test widget.shouldUpdateModel(state) is later mapped to getLatestValue() but the store.state there may already have changed due to another action.

[marcglasberg]

We inherited this bug from flutter_redux. I opened an issue there: https://github.com/brianegan/flutter_redux/issues/196

I did a detailed analysis of the architecture both projects use, and I'd say important changes must be made to solve this. Also the current architecture has performance problems, where the view-model is unnecessarily calculated more than it needs to (see https://github.com/brianegan/flutter_redux/issues/197).

I have already fixed the performance problems, but I'm still deciding how to fix the shouldUpdateModel bug without breaking changes. I should post some more details here soon.

[marcglasberg]

@kuhnroyal Could you please test it, and tell me if the problem is solved? You have to use the current version from GitHub:

  async_redux:
    git:
      url: https://github.com/marcglasberg/async_redux
      ref: 239284158a2f17a1a5e9e3188ce19e6c476461d4

[kuhnroyal]

@marcglasberg This looks good, thank you!

[kuhnroyal]

This works mostly but I still have a case where this happens. Here is some logging with comments. shouldUpdateModel returns false but the VM builder still gets that state (hash: 141413304) and runs into NoSuchMethodError in my case.

// shouldUpdateModel = false
flutter: shouldUpdateModel: 141413304 (state.hashCode)
flutter: null (some null check)
flutter: null (some null check)

// _shouldUpdateModel in _StoreStreamListenerState
flutter: storeHasValidModel: false
flutter: ifStreamHasValidModel: false
flutter: identical 
flutter: 372880470 (_mostRecentValidState.hashCode)

// VM builder
flutter: builder: 141413304  (state.hashCode)
flutter: null
flutter: null

I am not sure I can reproduce this in a simple test. Basically what I am doing is a logout procedure, clearing user state and removing all routes. The removed routes still get a rebuild with the wrong state. The shouldUpdateModel (see above) basically checks that user data is in the state and ClearUserDataAction removes the user state before logout. So in the above log the user data is gone and shouldUpdateModel returns false. I would expect the rebuild to happen with the _mostRecentValidState hash 372880470 but it gets state 141413304.

    await dispatchFuture(ClearUserDataAction());
    await dispatchFuture(PersistAction());
    await dispatchFuture(NavigateAction.pushNamedAndRemoveAll(AppRouter.login));

[marcglasberg]

If you go to store_connector.dart line 328 you have the relevant code:

  void _createStream() => _stream = widget.store.onChange
      // This prevents unnecessary calculations of the view-model.
      .where(_stateChanged)
      // Discards invalid states.
      .where(_shouldUpdateModel)
      // Calculates the view-model using the `vm` or `converter` functions.
      .map(_calculateModel)
      // Don't use `Stream.distinct` because it cannot capture the initial
      // ViewModel produced by the `converter`.
      .where(_whereDistinct)
      // Updates the latest-model with the calculated vm.
      // Important: This must be done after all other optional
      // transformations, such as shouldUpdateModel.
      .transform(StreamTransformer.fromHandlers(
        handleData: _handleData,
        handleError: _handleError,
      ));

Relevant methods are _shouldUpdateModel, _calculateModel and _handleData.

Could you please check that code and see if you spot anything?

Without a reproducible test this is quite hard to solve.

[kuhnroyal]

Without a reproducible test this is quite hard to solve.

I know, I will try to locate it.

[kuhnroyal]

We have just migrated a project from 3.0.5 to 6.0.3 and the same problem started happening there. I am gonna try with the new changes from 7.0.1 now.

[kuhnroyal]

I have tested this with 7.0.1 and it still has this problem. The _StoreStreamListenerState.didUpdateWidget is called and computes the latest model even when shouldUpdateModel returns false. I can not reproduce this in a simple example but I have 2 complex cases in 2 different project where this is happening.

  @override
  void didUpdateWidget(_StoreStreamListener<St, Model> oldWidget) {
    _computeLatestModel(); // This throws and uses a new state which shouldUpdateModel returned false for

    if (widget.store != oldWidget.store) {
      _createStream();
    }

    super.didUpdateWidget(oldWidget);
  }

[marcglasberg]

Yes, it would not have fixed itself, since I did not work in this for version 7. But it's on the radar. Ideally I should have a failing test with the StoreTester.

[kuhnroyal]

Oh didn't mean to pressure you, I thought due to the new currentState in the VmFactory that there may be changes. I would like to provide a failing test but I can't recreate it in simple examples.

And I am also not sure that both my problems have the same source. Because the first one came from the stream and the current one is from a StoreConnector rebuild.

[marcglasberg]

Don't worry, you are not pressuring me, you are helping. :)

The state and currentState in the VmFactory are just to make it clearer which state you are using, since this was a source of misunderstandings for some people.

[marcglasberg]

@kuhnroyal I believe this is fixed now, so I am closing this. The tests are passing, and in my own apps I use this a few different times, and they work well. Please let me know if you are still having problems. Thank you!

[kuhnroyal]

I am not on 12.0.0 in all projects but I will reopen if it pops up in the latest version. Thanks for your work on this project!

[kuhnroyal]

Still getting this in my VmFactories, more often on slow devices. The state is checked again shouldUpdateModel and it returns true but the factory then uses a state object that has changed in the meantime.

[kuhnroyal]

@marcglasberg I ran into this again today and tracked the problem to https://github.com/marcglasberg/async_redux/blame/master/lib/src/store_connector.dart#L408

Some external rebuild of the store connector causes a re-compute of the model from the store.state without considering the _forceLastValidStreamState in line https://github.com/marcglasberg/async_redux/blame/master/lib/src/store_connector.dart#L420

Maybe this needs to be changed to getLatestModel(_forceLastValidStreamState ?? widget.store.state)?

[kuhnroyal]

lol thats the same thing I figured out one and half years ago, I just noticed from my comment above....

[kuhnroyal]

Managed to replicate this now very easily by adapting the existing shouldUpdateModel sample: https://github.com/kuhnroyal/async_redux/commit/29f6fa02706c0421856329d73f74f230292ed843

Any external change, which may not be avoidable, can cause this to fail.