Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[React] ConcurrentModificationError #149

Closed
busslina opened this issue Apr 19, 2024 · 18 comments
Closed

[React] ConcurrentModificationError #149

busslina opened this issue Apr 19, 2024 · 18 comments

Comments

@busslina
Copy link

busslina commented Apr 19, 2024

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 busslina changed the title [React ReArch] [React ReArch] ConcurrentModificationError Apr 19, 2024
@busslina busslina changed the title [React ReArch] ConcurrentModificationError [React] ConcurrentModificationError Apr 19, 2024
@busslina
Copy link
Author

busslina commented Apr 19, 2024

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

@busslina
Copy link
Author

busslina commented Apr 19, 2024

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
Copy link
Owner

What’s the stack trace?

@busslina
Copy link
Author

busslina commented Apr 19, 2024

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
Copy link
Author

busslina commented Apr 19, 2024

Video demo:

1.mp4
2.mp4

@GregoryConrad
Copy link
Owner

GregoryConrad commented Apr 19, 2024

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
Copy link
Author

busslina commented Apr 19, 2024

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):

  • container listeners
  • dependency disposers
  • side effect data
  • ...

@busslina
Copy link
Author

busslina commented Apr 19, 2024

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
Copy link
Author

busslina commented Apr 19, 2024

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

imagen

@GregoryConrad
Copy link
Owner

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
Copy link
Owner

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

@busslina
Copy link
Author

I detected something weird caused on the react package: Workiva/react-dart#393

@busslina
Copy link
Author

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
Copy link
Author

busslina commented Apr 19, 2024

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 (Workiva/react-dart#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
Copy link
Author

busslina commented Apr 19, 2024

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
Copy link
Author

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
Copy link
Author

busslina commented Apr 19, 2024

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
Copy link
Author

busslina commented Apr 19, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants