vasturiano / react-force-graph

React component for 2D, 3D, VR and AR force directed graphs
https://vasturiano.github.io/react-force-graph/example/large-graph/
MIT License
2.26k stars 284 forks source link

Crash inside ForceGraph2d component #540

Open Ars25Terra opened 2 months ago

Ars25Terra commented 2 months ago

Hello, maybe you can help me.

I'm using ForceGraph2d with react.

It works fine, but occasionally (the case is not clear), when dragging nodes it crashes with following error:

TypeError: r is null _iterableToArray3 canvas-color-tracker.mjs:26 _toConsumableArray3 canvas-color-tracker.mjs:32 lookup canvas-color-tracker.mjs:97 hexIndex force-graph.mjs:1049 hexIndex force-graph.mjs:1047 onChange8 force-graph.mjs:1041 getSetProp kapsule.mjs:170 _call react-kapsule.mjs:125 index react-kapsule.mjs:134 index react-kapsule.mjs:133 React 12 workLoop scheduler.development.js:266 flushWork scheduler.development.js:239 performWorkUntilDeadline scheduler.development.js:533

Are there any advice or workarounds for this?
vasturiano commented 2 months ago

@Ars25Terra thanks for reaching out. It's not so easy to figure this out without a means to reproduce it. Was this happening in your app only? Are you able to make a simplified example on https://codesandbox.io/ which can reproduce this issue?

RomanKornev commented 2 months ago

@Ars25Terra @vasturiano I can replicate it pretty consistently in my local app (vite+react). It's a static graph with a search bar that's filtering the data. As I type, the graph is filtered and rerendered.

Some of the things I noticed:

I've managed to catch it when debugging and the __indexColor property is indeed null: https://github.com/vasturiano/force-graph/blob/34648d0464c84b1f22195513cb09fa9b3e6c5c4f/src/force-graph.js#L134-L138 If I modify the check above to

if (!d.hasOwnProperty('__indexColor') || d.__indexColor === null) return true;

then the problem disappears, but I am unsure of the implications of this change.

My guess would be that there is some race condition which is only visible when the CPU is choking when trying to process the data. But why does that never happen in production mode is a mystery to me.

Ars25Terra commented 2 months ago

@vasturiano, exactly what @RomanKornev mentions, but somehow I get __indexColor null also in production

Ars25Terra commented 2 months ago

@vasturiano unfortunately it's not easy to provide example on https://codesandbox.io/, but I did a deep research and found out the following.

__indexColor in force-graph becomes null, when registry in canvas-color-tracker is full. As far as I understood this happens on big graphs, as "register" function in canvas-color-tracker constantly pushes new objects with new indexColors and finally registry is filled. When registry is full it returns null for indexColor.

vasturiano commented 1 month ago

@Ars25Terra @RomanKornev thanks for your effort in figuring this out. The issue was indeed the mishandling of the color tracker registry getting full, leading to a null pointer exception.

I've just fixed this issue, in a manner similar to that suggested by @RomanKornev. If you upgrade your dependency tree, to get at least v1.44 of force-graph, you shouldn't experience the issue any longer. Please let me know if this solves it for you.

Ars25Terra commented 1 month ago

Dear @vasturiano, thank you a lot for the fix. Just one request, can you please update react-force-graph with new force-graph (1.44) dependency?

vasturiano commented 1 month ago

@Ars25Terra there's nothing really to upgrade in react-force-graph. This package has already the broad dependency match of "force-graph": "1".

https://github.com/vasturiano/react-force-graph/blob/ca22c59e62b8c715c4c273e405bf95b1b4ef08fc/package.json#L53C5-L53C24

So it should automatically pick up the new version of force-graph when your dependency tree is upgraded.

RomanKornev commented 1 month ago

I can confirm the issue is fixed. Thanks a lot for taking a look!

I assume this also fixes https://github.com/vasturiano/react-force-graph/issues/512

vasturiano commented 1 month ago

I can confirm the issue is fixed. Thanks a lot for taking a look!

I assume this also fixes #512

Glad we figured out this one. And yeah, that issue looks the same, so it's probably fixed too.