fix(): dispose-render concurrency

[ShaMan123]

Description

Fixes a race condition between requested rendering and disposing

Reproduce

1. requestRenderAll is called
2. dispose is called - frees resources (and tries to cancel the animation frame)
3. rendering is invoked from the requestAnimFrame callback
4. rendering tries to access a freed resource (ctx)
5. error

Changes

• rename isRendering => nextRenderHandle
• introduce destroy acting instead of dispose
• refactor dispose to call destroy after rendering is finished - it has become an async method that return true if disposing has completed, false if the canvas was already destroyed or throws an error if it was aborted by a consequent call
• introduced destroyed, disposed flag that blocks rendering calls from executing
• fix(): canvas#toCanvasElement used to kill requested renders b12491d
• added tests for edge cases
• extracted all canvas disposing tests to a dedicated file (I would have split it up more but we decided to do as less as possible to tests)
• adapt existing tests

replaces #8218

[github-actions]

Coverage after merging fix-dispose-render-concurrency into master will be

81.18%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
dist
fabric.js	79.54%	75.73%	84.83%	81.18%	10053, 10072–10074, 10076–10077, 10139, 10193, 10208, 10264, 10300–10301, 10307, 10311–10312, 10327, 10361, 10392–10393, 10417, 10425, 10512, 10544–10545, 10620–10621, 10624, 10629, 10651, 10651, 10651, 10651, 10651, 10651, 10651–10652, 10654–10655, 10655, 10655–10656, 10661, 10663, 10669, 10669, 10669, 10669, 10669–10670, 10672–10676, 10676, 10676–10678, 10680–10681, 10690, 10701, 10712, 10723, 10733–10736, 10744–10745, 10745, 10745–10746, 10748–10749, 10756, 10760, 1085, 11357, 11362, 11404–11406, 11406, 11406, 11406, 11406, 11406–11407, 11427, 11432–11433, 11473, 11473, 11473, 11473, 11501, 11501, 11501, 11503, 116, 1160, 1162–1163, 1165, 11651–11652, 1166, 11663, 1173, 1177, 11820–11821, 11828, 1185, 11871, 11878, 11878, 11887–11888, 11899, 119, 11939, 11947, 11966–11968, 11995–11996, 11999, 11999, 120, 12016–12018, 121, 12121, 12206–12207, 12209, 1221, 12210, 12217, 1223, 1223, 1223, 1223, 1223–1224, 12241, 12307–12308, 12312, 12392, 12403, 12408, 12445, 12546, 12546, 12546, 12570–12571, 12679, 12682, 12744, 12750, 12757, 12764, 12770, 12776, 12783, 12790, 12796–12797, 12797, 12797, 12812–12813, 12821, 12830, 12830, 12855–12858, 12897, 129, 12940–12941, 12981–12982, 130–131, 13115–13116, 13261, 13353, 13416, 13419, 13472–13473, 13473, 13473, 13476, 13491, 13505, 13517–13518, 13520, 13532–13533, 13535, 13550, 13565–13566, 13568–13569, 13571–13572, 13582, 13612, 13612, 13612, 13612, 13612, 13612, 13612, 13612, 13637, 13639, 13639–13641, 13694–13696, 13719, 13727, 13733–13734, 13775, 13838–13839, 13873, 13894–13895, 1390, 1390, 1392, 13954, 13954, 13954, 13954, 13954, 13959–13967, 14009, 14135–14136, 14172, 14181, 14202, 14202, 14202–14203, 14203, 14203, 14203, 14203–14204, 14210–14212, 14215–14216, 14229, 14229, 14229–14230, 14230, 14230, 14230, 14230–14231, 14237–14239, 14242–14243, 14256–14257, 14331, 14380, 14384, 1442, 14509, 14539–14540, 14540, 14540–14541, 14541, 14541–14542, 14544, 14544, 14544–14545, 14548, 14555, 14640, 14753, 14802, 14872, 14876, 14946, 15003–15004, 15018, 15068–15069, 15071–15072, 15164, 15224, 15227, 15302, 15305, 15320–15321, 15330, 15372, 15376, 15401–15402, 15436, 15440, 1562, 15706, 15710, 15874, 15877, 15884–15885, 15885, 15885–15887, 15889, 15889, 15889–15891, 15891, 15891–15893, 15983, 15988–15990, 16018–16019, 16086, 16089, 16089, 16089–16090, 16092–16093, 16093, 16093–16094, 16094, 16094, 16096–16097, 16099, 16102, 16129, 16129–16131, 16180, 16180, 16215, 1623, 16250, 16250, 16250, 16253–16254, 16256, 16256, 16260, 16263, 16266, 16268–16269, 16278, 16283, 16283–16285, 16285, 16296–16297, 16306, 16306, 16306, 16306, 16306–16307, 16307, 16307–16309, 16309–16310, 16310, 16327–16328, 16328, 16328, 16350–16351, 16351, 16351, 16351, 16351, 16351, 16363, 16363, 16366–16370, 16370, 16409, 16448, 16480, 16483–16486, 16496, 16509, 16524, 16541, 16550, 16554, 16606, 16608–16610, 16624, 16626–16627, 16643, 16695, 16718, 16851, 16914–16916, 16916–16917, 16917, 16945, 16990–16993, 17000, 17002–17003, 17018–17020, 17030, 17030, 17030, 17093, 17105, 17105, 17152–17153, 17171–17172, 17193, 17234, 17234–17235, 17252–17255, 17255, 17255–17256, 17258, 17258, 17258–17259, 17261–17262, 17262, 17262–17263, 17265, 17265, 17265–17266, 17268–17269, 17273–17274, 17321, 17365, 17434, 17439, 17463–17464, 17473–17477, 17489–17492, 17497, 17506–17507, 17509, 17518, 17518, 17518, 17518, 17518–17519, 17521–17522, 17538–17539, 17541–17542, 17549–17552, 17555, 17558, 17560–17561, 17561, 17561, 17561, 17561, 17561, 17561–17562, 17564, 17566–17567, 17567, 17567–17569, 1757, 17570, 17572, 17579, 1758, 17580–17587, 17587, 17587–17589, 17592, 17600–17603, 17609–17610, 17610, 17610–17611, 17613, 17613, 17613–17614, 17616, 17618–17619, 17634, 17636, 17636, 17636–17637, 17639–17640, 17640–17641, 17641, 17647, 17647, 17649, 17649–17650, 17650, 17659–17661, 17661, 17661–17669, 17675, 17675, 17675–17677, 17679, 17685–17686, 17700–17706, 17706, 17706–17707, 17710, 17712, 17724, 17724, 17724–17725, 17728–17730, 17740, 17740, 17740–17742, 17754, 17754, 17754–17755, 17757–17758, 17758, 17758–17759, 17761–17762, 17762, 17762–17765, 17765, 17765–17766, 17768, 17768, 17768–17769, 17772–17773, 17776, 17778–17779, 17779, 17779, 17779, 17779–17781, 17795–17797, 17799–17800, 17811, 17813, 17815–17818, 17875, 17941, 17941, 17941–17942, 17942, 17942–17943, 17943,

[ShaMan123]

Viewed the test logs
It is fixed!

[ShaMan123]

need to add more tests

DONE

[ShaMan123]

We need to document that now if animations aren't aborted the canvas will never dispose

done b56685d

[ShaMan123]

Updated description
Added extensive tests
Ready!!

[ShaMan123]

I earned 0.1% of testing coverage in this PR 🤓

[asturur]

What are we doing with those?
i see you want to resolve with true/false in case this is an effective dispose or not.
But why? what does it give us this extra information?

[ShaMan123]

Perhaps someone needs to wait until dispose is done to trigger some task that shouldn't start if not disposed

[asturur]

This seems good to me and less invasive than the original.
I just need some time to read the tests, but definitely the code is good.
Just unclear for me that difference between disposed and destroyed and on top of that i wonder, should a render request started before a dispose call end correctly?
I would say yes, but we skip it if we are disposing.
Thoughts?

[ShaMan123]

I wanted to abort the rendering once disposed is called. That is why I tried the first approach.
For me it makes less sense waiting for the render to finish. But it doesn't matter too much.
I am not sure I understood what you meant

I would say yes, but we skip it if we are disposing.

What happens now with useEffect initializing the canvas? It will try to dispose the canvas, but that is async and will tear the hook cycle apart, won't it? If it tries to dispose, and then hot reload we will get a multiple canvas init error I think.

destroy is the actual logic that kills the canvas (should be protected). dispose is only in charge of calling destroy safely.
Subclass should override destroy only.

[ShaMan123]

I would say yes, but we skip it if we are disposing.

You mean maybe we should let the rendering happen even if the the disposed flag is set?
We can't. 'Cause if there's an infinite animation the concurrency error will surface.

[ShaMan123]

What if we wrap cancelAnimFrame so if it is called it flags something for requestAnimFrame so in case the timeout is > 16ms it will not invoke the callback related to the handleID?

[ShaMan123]

What happens now with useEffect initializing the canvas? It will try to dispose the canvas, but that is async and will tear the hook cycle apart, won't it? If it tries to dispose, and then hot reload we will get a multiple canvas init error I think.

This makes me prefer the previous approach

[ShaMan123]

@asturur please comment on the following before we proceed

What happens now with useEffect initializing the canvas?

Let's say we trigger a hot reload. useEffect will try to dispose the canvas but that is async. Will useEffect reinitialize the canvas before dispose will complete? If so we will get a multiple canvas init error

I have to say that the prev approach is legit. You read it but I refactored it afterwards.

[ShaMan123]

21246b2 safeguards renderAll from running after disposed, something that could happen in infinite animation

[asturur]

@asturur please comment on the following before we proceed

What happens now with useEffect initializing the canvas?

Let's say we trigger a hot reload. useEffect will try to dispose the canvas but that is async. Will useEffect reinitialize the canvas before dispose will complete? If so we will get a multiple canvas init error

I have to say that the prev approach is legit. You read it but I refactored it afterwards.

I m not sure what will happen with a react binding made to dispose a canvas on element unmount for an hot reload.
That depends on the app and react hook setup. Can useEffect wait an effect to resolve?

Also, if you are disposing a canvas, you should not reinitialize the same. Dispose is mostly to trash the instance and clean te resources. If you dispose on unmount, you are probably adding a new instance on mount?

[ShaMan123]

Correct.
But adding a new instance on the same canvas element before it is disposed will throw an "already initialized canvas" error.
I am not sure if useEffect can wait or not, nothing in react docs saying so