From 7538aeae0ef1aa227e975a06965030135fc6b7b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= Date: Tue, 27 Jun 2023 09:27:52 +0200 Subject: [PATCH] [IMP] runtime: only destroy component in raf callback Before this commit, most of the time, components are destroyed when the virtual dom is patched and a component node is removed. However, since Owl is asynchronous and a component may take some time to get ready (with onWillStart), it can happen that a component is created, then before it is ready, it is recreated. In that case, the initial instance has to be destroyed. Before this commit, the destroy operation was done immediately, when we cancel the current fibers. However, this means that we cannot have a guarantee between micro task ticks that a component has not been destroyed in the meantime. For example, in Odoo, it is common to use the rpc service, which will throw an error if called by a destroyed component. But because of the possible destruction of a component at any time, the following code is unsafe: async loadSomeData() { // guaranteed to be called when component is alive await Promise.resolve(); // however here, component may have been destroyed this.rpc(...) } So, to prevent this issue, we can slightly delay the destroy operation. It is not entirely trivial, since we need to find a way to neutralize the component in the meantime. But it seems like performing all that kind of operation at the "commit" phase (so, the request animation frame callback) makes sense to me. So, this commit modifies the code to add a new component status (cancelled) and use it to cancel components that are waiting to be destroyed. These components will then be destroyed as soon as the requestanimation frame starts, before all other dom operations. --- doc/reference/component.md | 1 + src/runtime/component_node.ts | 19 ++- src/runtime/error_handling.ts | 20 +-- src/runtime/fibers.ts | 3 +- src/runtime/scheduler.ts | 35 +++-- src/runtime/status.ts | 7 +- .../__snapshots__/concurrency.test.ts.snap | 67 +++++++++ tests/components/concurrency.test.ts | 141 +++++++++++++++--- tests/components/error_handling.test.ts | 8 +- 9 files changed, 259 insertions(+), 42 deletions(-) diff --git a/doc/reference/component.md b/doc/reference/component.md index 6d8331f3..0fed57ae 100644 --- a/doc/reference/component.md +++ b/doc/reference/component.md @@ -451,5 +451,6 @@ console.log(status(component)); // logs either: // - 'new', if the component is new and has not been mounted yet // - 'mounted', if the component is currently mounted +// - 'cancelled', if the component has not been mounted yet but will be destroyed soon // - 'destroyed' if the component is currently destroyed ``` diff --git a/src/runtime/component_node.ts b/src/runtime/component_node.ts index 62128757..98f580a4 100644 --- a/src/runtime/component_node.ts +++ b/src/runtime/component_node.ts @@ -145,6 +145,9 @@ export class ComponentNode

implements VNode= STATUS.CANCELLED) { + return; + } let current = this.fiber; if (current && (current.root!.locked || (current as any).bdom === true)) { await Promise.resolve(); @@ -171,7 +174,7 @@ export class ComponentNode

implements VNode= STATUS.CANCELLED) { return; } // We only want to actually render the component if the following two @@ -190,6 +193,20 @@ export class ComponentNode

implements VNode = new Set(); constructor() { this.requestAnimationFrame = Scheduler.requestAnimationFrame; @@ -23,6 +25,13 @@ export class Scheduler { this.tasks.add(fiber.root!); } + scheduleDestroy(node: ComponentNode) { + this.cancelledNodes.add(node); + if (this.frame === 0) { + this.frame = this.requestAnimationFrame(() => this.processTasks()); + } + } + /** * Process all current tasks. This only applies to the fibers that are ready. * Other tasks are left unchanged. @@ -39,15 +48,23 @@ export class Scheduler { } if (this.frame === 0) { - this.frame = this.requestAnimationFrame(() => { - this.frame = 0; - this.tasks.forEach((fiber) => this.processFiber(fiber)); - for (let task of this.tasks) { - if (task.node.status === STATUS.DESTROYED) { - this.tasks.delete(task); - } - } - }); + this.frame = this.requestAnimationFrame(() => this.processTasks()); + } + } + + processTasks() { + this.frame = 0; + for (let node of this.cancelledNodes) { + node._destroy(); + } + this.cancelledNodes.clear(); + for (let task of this.tasks) { + this.processFiber(task); + } + for (let task of this.tasks) { + if (task.node.status === STATUS.DESTROYED) { + this.tasks.delete(task); + } } } diff --git a/src/runtime/status.ts b/src/runtime/status.ts index 9f48552c..e0d74c62 100644 --- a/src/runtime/status.ts +++ b/src/runtime/status.ts @@ -7,15 +7,20 @@ import type { Component } from "./component"; export const enum STATUS { NEW, MOUNTED, // is ready, and in DOM. It has a valid el + // component has been created, but has been replaced by a newer component before being mounted + // it is cancelled until the next animation frame where it will be destroyed + CANCELLED, DESTROYED, } -type STATUS_DESCR = "new" | "mounted" | "destroyed"; +type STATUS_DESCR = "new" | "mounted" | "cancelled" | "destroyed"; export function status(component: Component): STATUS_DESCR { switch (component.__owl__.status) { case STATUS.NEW: return "new"; + case STATUS.CANCELLED: + return "cancelled"; case STATUS.MOUNTED: return "mounted"; case STATUS.DESTROYED: diff --git a/tests/components/__snapshots__/concurrency.test.ts.snap b/tests/components/__snapshots__/concurrency.test.ts.snap index a430f6c2..a3c86491 100644 --- a/tests/components/__snapshots__/concurrency.test.ts.snap +++ b/tests/components/__snapshots__/concurrency.test.ts.snap @@ -212,6 +212,73 @@ exports[`changing state before first render does not trigger a render 1`] = ` }" `; +exports[`component destroyed just after render 1`] = ` +"function anonymous(app, bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, comment } = bdom; + const comp1 = app.createComponent(\`B\`, true, false, false, []); + + return function template(ctx, node, key = \\"\\") { + return comp1({}, key + \`__1\`, node, this, null); + } +}" +`; + +exports[`component destroyed just after render 2`] = ` +"function anonymous(app, bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, comment } = bdom; + + return function template(ctx, node, key = \\"\\") { + const b2 = text(\`B\`); + const b3 = text(ctx['state'].value); + return multi([b2, b3]); + } +}" +`; + +exports[`components are not destroyed between animation frame 1`] = ` +"function anonymous(app, bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, comment } = bdom; + const comp1 = app.createComponent(\`B\`, true, false, false, []); + + return function template(ctx, node, key = \\"\\") { + let b2,b3; + b2 = text(\`A\`); + if (ctx['state'].flag) { + b3 = comp1({}, key + \`__1\`, node, this, null); + } + return multi([b2, b3]); + } +}" +`; + +exports[`components are not destroyed between animation frame 2`] = ` +"function anonymous(app, bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, comment } = bdom; + const comp1 = app.createComponent(\`C\`, true, false, false, []); + + return function template(ctx, node, key = \\"\\") { + const b2 = text(\`B\`); + const b3 = comp1({}, key + \`__1\`, node, this, null); + return multi([b2, b3]); + } +}" +`; + +exports[`components are not destroyed between animation frame 3`] = ` +"function anonymous(app, bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, comment } = bdom; + + return function template(ctx, node, key = \\"\\") { + return text(\`C\`); + } +}" +`; + exports[`concurrent renderings scenario 1 1`] = ` "function anonymous(app, bdom, helpers ) { diff --git a/tests/components/concurrency.test.ts b/tests/components/concurrency.test.ts index c23ff6dc..62909302 100644 --- a/tests/components/concurrency.test.ts +++ b/tests/components/concurrency.test.ts @@ -115,13 +115,7 @@ test("destroying/recreating a subwidget with different props (if start is not ov await nextMicroTick(); expect(n).toBe(2); - expect([ - "Child:willDestroy", - "W:willRender", - "Child:setup", - "Child:willStart", - "W:rendered", - ]).toBeLogged(); + expect(["W:willRender", "Child:setup", "Child:willStart", "W:rendered"]).toBeLogged(); def.resolve(); await nextTick(); @@ -130,6 +124,7 @@ test("destroying/recreating a subwidget with different props (if start is not ov expect([ "Child:willRender", "Child:rendered", + "Child:willDestroy", "W:willPatch", "Child:mounted", "W:patched", @@ -178,13 +173,13 @@ test("destroying/recreating a subcomponent, other scenario", async () => { "Child:setup", "Child:willStart", "Parent:rendered", - "Child:willDestroy", "Parent:willRender", "Child:setup", "Child:willStart", "Parent:rendered", "Child:willRender", "Child:rendered", + "Child:willDestroy", "Parent:willPatch", "Child:mounted", "Parent:patched", @@ -251,13 +246,13 @@ test("creating two async components, scenario 1", async () => { await nextTick(); expect(fixture.innerHTML).toBe(""); expect([ - "ChildA:willDestroy", "Parent:willRender", "ChildA:setup", "ChildA:willStart", "ChildB:setup", "ChildB:willStart", "Parent:rendered", + "ChildA:willDestroy", ]).toBeLogged(); defB.resolve(); @@ -703,13 +698,13 @@ test("rendering component again in next microtick", async () => { "Child:setup", "Child:willStart", "Parent:rendered", - "Child:willDestroy", "Parent:willRender", "Child:setup", "Child:willStart", "Parent:rendered", "Child:willRender", "Child:rendered", + "Child:willDestroy", "Parent:willPatch", "Child:mounted", "Parent:patched", @@ -1732,9 +1727,9 @@ test("concurrent renderings scenario 10", async () => { expect(fixture.innerHTML).toBe("

"); expect([ "ComponentA:willRender", - "ComponentC:willDestroy", "ComponentB:willUpdateProps", "ComponentA:rendered", + "ComponentC:willDestroy", ]).toBeLogged(); defB.resolve(); @@ -2282,7 +2277,6 @@ test("concurrent renderings scenario 16", async () => { "D:setup", "D:willStart", "C:rendered", - "D:willDestroy", "B:willRender", "C:willUpdateProps", "B:rendered", @@ -2290,6 +2284,7 @@ test("concurrent renderings scenario 16", async () => { "D:setup", "D:willStart", "C:rendered", + "D:willDestroy", ]).toBeLogged(); // at this point, C rendering is still pending, and nothing should have been @@ -2997,11 +2992,11 @@ test("t-key on dom node having a component", async () => { expect(fixture.innerHTML).toBe("
3
"); expect([ - "Child (2):willDestroy", "Child (3):setup", "Child (3):willStart", "Child (3):willRender", "Child (3):rendered", + "Child (2):willDestroy", "Child (1):willUnmount", "Child (1):willDestroy", "Child (3):mounted", @@ -3055,11 +3050,11 @@ test("t-key on dynamic async component (toggler is never patched)", async () => expect(fixture.innerHTML).toBe("
3
"); expect([ - "Child (2):willDestroy", "Child (3):setup", "Child (3):willStart", "Child (3):willRender", "Child (3):rendered", + "Child (2):willDestroy", "Child (1):willUnmount", "Child (1):willDestroy", "Child (3):mounted", @@ -3114,11 +3109,11 @@ test("t-foreach with dynamic async component", async () => { expect(fixture.innerHTML).toBe("
3
"); expect([ - "Child (2):willDestroy", "Child (3):setup", "Child (3):willStart", "Child (3):willRender", "Child (3):rendered", + "Child (2):willDestroy", "Child (1):willUnmount", "Child (1):willDestroy", "Child (3):mounted", @@ -3801,7 +3796,7 @@ test("destroyed component causes other soon to be destroyed component to rerende static template = xml``; state = useState({ val: 0 }); setup() { - c = this; + c = c || this; useLogLifecycle(); } } @@ -3846,8 +3841,6 @@ test("destroyed component causes other soon to be destroyed component to rerende parent.state.valueB = 2; await nextTick(); expect([ - "B:willDestroy", - "C:willDestroy", "A:willRender", "B:setup", "B:willStart", @@ -3858,6 +3851,8 @@ test("destroyed component causes other soon to be destroyed component to rerende "B:rendered", "C:willRender", "C:rendered", + "B:willDestroy", + "C:willDestroy", "A:willPatch", "C:mounted", "B:mounted", @@ -4200,6 +4195,116 @@ test("delayed render is not cancelled by upcoming render", async () => { ]).toBeLogged(); }); +test("components are not destroyed between animation frame", async () => { + const def = makeDeferred(); + class C extends Component { + static template = xml`C`; + setup() { + useLogLifecycle(); + } + } + class B extends Component { + static template = xml`B`; + static components = { C }; + setup() { + useLogLifecycle(); + onWillStart(() => { + return def; + }); + } + } + class A extends Component { + static template = xml`A`; + static components = { B }; + + state = useState({ flag: false }); + setup() { + useLogLifecycle(); + } + } + const a = await mount(A, fixture); + expect(fixture.innerHTML).toBe("A"); + expect(["A:setup", "A:willStart", "A:willRender", "A:rendered", "A:mounted"]).toBeLogged(); + + // turn the flag on, this will render A and stops at B because of def + a.state.flag = true; + await nextTick(); + expect(["A:willRender", "B:setup", "B:willStart", "A:rendered"]).toBeLogged(); + + // force a render of A + // => owl will need to create a new B component + // => initial B component will be cancelled + a.render(); + await nextMicroTick(); + expect([ + // note that B is not destroyed here. It is cancelled instead + "A:willRender", + "B:setup", + "B:willStart", + "A:rendered", + ]).toBeLogged(); + + // resolve def, so B render is unblocked + def.resolve(); + await nextTick(); + expect([ + "B:willRender", + "C:setup", + "C:willStart", + "B:rendered", + "C:willRender", + "C:rendered", + // animation frame callback starts here + "B:willDestroy", // B is destroyed here + "A:willPatch", + "C:mounted", + "B:mounted", + "A:patched", + ]).toBeLogged(); +}); + +test("component destroyed just after render", async () => { + let stateB: any; + + class B extends Component { + static template = xml`B`; + state = useState({ value: 1 }); + setup() { + stateB = this.state; + useLogLifecycle(); + } + } + class A extends Component { + static template = xml``; + static components = { B }; + setup() { + useLogLifecycle(); + } + } + const a = await mount(A, fixture); + expect(fixture.innerHTML).toBe("B1"); + expect([ + "A:setup", + "A:willStart", + "A:willRender", + "B:setup", + "B:willStart", + "A:rendered", + "B:willRender", + "B:rendered", + "B:mounted", + "A:mounted", + ]).toBeLogged(); + + stateB!.value++; // force a render of B + await nextMicroTick(); // wait for B render to actually start + a.__owl__.app.destroy(); + expect(["A:willUnmount", "B:willUnmount", "B:willDestroy", "A:willDestroy"]).toBeLogged(); + await nextTick(); + // check that B was not rendered after being destroyed + expect([]).toBeLogged(); +}); + // test.skip("components with shouldUpdate=false", async () => { // const state = { p: 1, cc: 10 }; diff --git a/tests/components/error_handling.test.ts b/tests/components/error_handling.test.ts index e78230e3..a24d4f9e 100644 --- a/tests/components/error_handling.test.ts +++ b/tests/components/error_handling.test.ts @@ -1444,6 +1444,7 @@ describe("can catch errors", () => { "Parent:willPatch", "Child:willUnmount", "Child:willDestroy", + "Parent:patched", "Parent:willRender", "Parent:rendered", "Parent:willPatch", @@ -1506,12 +1507,15 @@ describe("can catch errors", () => { parent.state.hasChild = false; await nextTick(); expect([ + "Parent:willRender", + "Parent:rendered", "Child:willDestroy", "Parent:willRender", "Parent:rendered", - "Parent:willPatch", - "Parent:patched", ]).toBeLogged(); + expect(fixture.innerHTML).toBe("1"); + await nextTick(); + expect(["Parent:willPatch", "Parent:patched"]).toBeLogged(); expect(fixture.innerHTML).toBe("2"); }); });