GregoryConrad / rearch-dart

Re-imagined approach to application design and architecture
https://pub.dev/packages/rearch
MIT License
92 stars 4 forks source link

[React] ConcurrentModificationError #149

Closed busslina closed 7 months ago

busslina commented 7 months ago

I'm getting an error with this code inside a RearchConsumer.build method on the new React port. It works well in the Flutter counterpart

@override
wlib.ReactNode? build(wlib.ComponentHandle use) {
  final loadingController = use(loadingCapsule);

  use.effect(
    () {
      Timer(const Duration(seconds: 10), () {
        loadingCtrl.off();
      });

      return null;
    },
    [],
  );

  return (div(
    {},
    button(
      {
        'onClick': (_) => loadingCtrl.off(),
      },
      'Button',
    ),
  ));
}

It fails after 10 seconds on the Timer callback execution.

Have you any idea on why it's happening?

It fails too with callonce + (Timer or Future.delayed). It's weird because it works well when triggered by a button click, which should produce an identical outcome as they are equally async...

busslina commented 7 months ago

~The only reason I can think of is the absence of Bootstrap logic, so I will create it and test it.~

busslina commented 7 months ago

~The only reason I can think of is the absence of Bootstrap logic, so I will create it and test it.~

No, bootstrap has nothing about this. It has to be something else.

GregoryConrad commented 7 months ago

What’s the stack trace?

busslina commented 7 months ago

What’s the stack trace?

Sorry, I removed it accidentally when editing the message.

Uncaught 
Object { Symbol("ConcurrentModificationError.modifiedObject"): {…} }
[collection_patch.dart:574:8](https://_____/lib/_internal/js_dev_runtime/patch/collection_patch.dart)
next collection_patch.dart:574
dispose impl.dart:63
buildNodesAndDependents node.dart:47
runTransaction rearch.dart:137
runTransaction impl.dart:108
busslina commented 7 months ago

Video demo:

https://github.com/GregoryConrad/rearch-dart/assets/73592852/22395f93-d21b-4df3-b913-ebd978ca7aee

https://github.com/GregoryConrad/rearch-dart/assets/73592852/9f361f89-4016-4313-8ff9-6c1adaa402b3

GregoryConrad commented 7 months ago

A dispose function is being registered on the capsule (or maybe a listener/onNextUpdate) while the capsule is being disposed. Can perhaps look into it more later, but if this exact same scenario works as expected over in the flutter package, I’d take a look at your usages of any container listeners or onNextUpdate calls that may add in a dispose listener to a capsule that may be getting disposed

busslina commented 7 months ago

Btw I just made private a bunch of Consumer stuff (which I don't know why they are public on the Flutter implementation if they can be private):

busslina commented 7 months ago

It isn't executing registerDispose() when the error happened. The 'red marked' is executed on the first build:

imagen

Gonna check if componentDidUpdate is disposing and recreating a new Component... (It shouldn't)

busslina commented 7 months ago

Detected two creations when it should be only one, of the top level component:

imagen

GregoryConrad commented 7 months ago

which I don't know why they are public on the Flutter implementation if they can be private

id have the check the code to be sure, but I believe that’s because they’re defined on a package or library-private class, I.e. they aren’t apart of the public API anyways

GregoryConrad commented 7 months ago

The issue is a call to registerDispose somewhere I believe, being called on a capsule’s (whether user-defined or temporary) disposal

busslina commented 7 months ago

I detected something weird caused on the react package: https://github.com/Workiva/react-dart/issues/393

busslina commented 7 months ago

id have the check the code to be sure, but I believe that’s because they’re defined on a package or library-private class, I.e. they aren’t apart of the public API anyways

You are right. On the flutter implementation they are on a private class. On the react one, on a public class.

busslina commented 7 months ago

I simplified the example to this case test: just one component, one capsule, a button and a Timer. So now you can test it easier by installing and executing webdev (see README.md).

I'm not sure whether this issue (https://github.com/Workiva/react-dart/issues/393) can be the cause of this unexpected behaviour. I compared the flutter and react implementations and they are pretty the same.

imagen

Image description: Two bootstrappers created, and two apps created (on one render call). For now I cannot just create one component only (always a minimum of two)

busslina commented 7 months ago

I'm thinking of doing a react fork because it seems abandoned. Do you think it worth it? In other words do you think our issue can be related to their issue?

UPDATE Forked: https://github.com/busslina/react-dart

busslina commented 7 months ago

I tested again with my react fork, now I'm able to prevent executing ReArch internals by components created by react but not inserted in the Component tree....

But the error isn't there. Every render calls _clearDependencies like the flutter_rearch build does, then the error came.

// Clears the old dependencies (which will be repopulated via WidgetHandle)
_clearDependencies();

imagen

componentDidUpdate is just logging, but not doing anything more. But something that can be weird is that the error seems not happening just in _clearDependencies, because as you can see, componentDidUpdate appears "in the middle".

When triggered by the button click, the execution flow seems the same, but not failing... :

imagen

busslina commented 7 months ago

I think I got what is happening: Flutter lib is calling rebuild async and react lib sync. Gonna try to to async too:

imagen

And also I realized that the flag bool _building is useless because render is not async, so nobody will execute another code at the middle of it.

busslina commented 7 months ago

Solved.

Solution: - Replacing sync forceUpdate() call on Api.rebuild() by _markNeedsBuild() async logic. - Removing unnecessary _building flag. - Calling _clearNeedsBuild() in componentDidUpdate() instead of in render()

Also, this issue helped to detect a non-fatal issue on react lib: For every Component2 registered, it creates a first instance of it "useless" (meaning by useless that is not inserted in the Component Tree). Is non-fatal because it doesn't execute lifecycle methods so it doesn't interact with ReArch implementation.