From 040c73fe694376825ff38de315b1ff6a3c5b8eed Mon Sep 17 00:00:00 2001 From: Aaron Bohy Date: Fri, 17 Sep 2021 11:06:44 +0000 Subject: [PATCH] [FIX] web: fix concurrency issue in graph Let's assume the following scenario in a graph view - toggle a filter on a slow network s.t. it takes a while to load - while it's loading, toggle a measure Before this commit, the view was kind of locked after those two steps, i.e. toggling measures or changing graph mode did not work anymore (it could be unlocked by interacting again with the search view though). The reason is that the new filter triggered a re-rendering of the withSearch component (the parent of the graph view), thus calling willUpdateProps on the graph view, which reloads the data. Toggling a measure cancels the current load operation (via keepLast), and the promise returned by willUpdateProps is thus pending forever. From that point, only another re-rendering from the parent (withSearch) could unlock the situation by cancelling the previous rendering. To solve this, we use the new Race primitive. It ensures that the promise returned by load resolves as soon as the view is actually reloaded (whether it came from the load itself, of by toggling a measure). This commit also refactors the pivot model to use the Race primitive. X-original-commit: ceebab7128cb9838024029aefe3910a18cf14e44 Part-of: odoo/odoo#76778 --- .../web/static/src/views/graph/graph_model.js | 25 ++-- .../web/static/src/views/pivot/pivot_model.js | 74 +++++------ .../static/tests/views/graph_view_tests.js | 116 +++++++++++++++++- 3 files changed, 158 insertions(+), 57 deletions(-) diff --git a/addons/web/static/src/views/graph/graph_model.js b/addons/web/static/src/views/graph/graph_model.js index 909fce47610..a784104520d 100644 --- a/addons/web/static/src/views/graph/graph_model.js +++ b/addons/web/static/src/views/graph/graph_model.js @@ -1,7 +1,7 @@ /** @odoo-module **/ import { sortBy } from "@web/core/utils/arrays"; -import { KeepLast } from "@web/core/utils/concurrency"; +import { KeepLast, Race } from "@web/core/utils/concurrency"; import { rankInterval } from "@web/search/utils/dates"; import { getGroupBy } from "@web/search/utils/group_by"; import { GROUPABLE_TYPES } from "@web/search/utils/misc"; @@ -85,7 +85,13 @@ export class GraphModel extends Model { * @override */ setup(params) { + // concurrency management this.keepLast = new KeepLast(); + this.race = new Race(); + const _fetchDataPoints = this._fetchDataPoints.bind(this); + this._fetchDataPoints = (...args) => { + return this.race.add(_fetchDataPoints(...args)); + }; this.initialGroupBy = null; @@ -126,9 +132,7 @@ export class GraphModel extends Model { metaData.additionalMeasures ); - await this._fetchDataPoints(metaData); - - this._prepareData(); + return this._fetchDataPoints(metaData); } /** @@ -144,17 +148,13 @@ export class GraphModel extends Model { * @param {Object} params */ async updateMetaData(params) { - const metaData = Object.assign({}, this.metaData, params); - if ("measure" in params) { - await this._fetchDataPoints(metaData); + await this._fetchDataPoints(Object.assign({}, this.metaData, params)); } else { - await this.keepLast.add(Promise.resolve()); - this.metaData = metaData; + await this.race.getCurrentProm(); + this.metaData = Object.assign({}, this.metaData, params); + this._prepareData(); } - - this._prepareData(); - this.notify(); } @@ -171,6 +171,7 @@ export class GraphModel extends Model { async _fetchDataPoints(metaData) { this.dataPoints = await this.keepLast.add(this._loadDataPoints(metaData)); this.metaData = metaData; + this._prepareData(); } /** diff --git a/addons/web/static/src/views/pivot/pivot_model.js b/addons/web/static/src/views/pivot/pivot_model.js index 68a4ef04b6a..fe30557458b 100644 --- a/addons/web/static/src/views/pivot/pivot_model.js +++ b/addons/web/static/src/views/pivot/pivot_model.js @@ -2,7 +2,7 @@ import { Domain } from "@web/core/domain"; import { cartesian, sections, sortBy, symmetricalDifference } from "@web/core/utils/arrays"; -import { KeepLast } from "@web/core/utils/concurrency"; +import { KeepLast, Race } from "@web/core/utils/concurrency"; import { computeVariation } from "@web/core/utils/numbers"; import { DEFAULT_INTERVAL } from "@web/search/utils/dates"; import { Model } from "@web/views/helpers/model"; @@ -338,7 +338,13 @@ export class PivotModel extends Model { * @param {Object} [params.data] previously exported data */ setup(params) { + // concurrency management this.keepLast = new KeepLast(); + this.race = new Race(); + const _loadData = this._loadData.bind(this); + this._loadData = (...args) => { + return this.race.add(_loadData(...args)); + }; let sortedColumn = params.metaData.sortedColumn || null; if (!sortedColumn && params.metaData.defaultOrder) { @@ -373,9 +379,6 @@ export class PivotModel extends Model { this.metaData = this._buildMetaData(metaData); this.reload = false; // used to discriminate between the first load and subsequent reloads - // loadProm ensures that loadData always returns a promise that resolves when - // the final data is loaded (in case several load requests arrive consecutively) - this.loadProm = null; this.nextActiveMeasures = null; // allows to toggle several measures consecutively } @@ -394,7 +397,7 @@ export class PivotModel extends Model { * @param {string} [params.interval] */ async addGroupBy(params) { - if (this.loadProm) { + if (this.race.getCurrentProm()) { return; // we are currently reloaded the table } @@ -435,7 +438,7 @@ export class PivotModel extends Model { * @param {'row'|'col'} type */ closeGroup(groupId, type) { - if (this.loadProm) { + if (this.race.getCurrentProm()) { return; // we are currently reloading the table } @@ -507,7 +510,7 @@ export class PivotModel extends Model { * @param {'row'|'col'} type */ async expandGroup(groupId, type) { - if (this.loadProm) { + if (this.race.getCurrentProm()) { return; // we are currently reloaded the table } @@ -598,7 +601,7 @@ export class PivotModel extends Model { * flipping the axes. This method is thus async. */ async flip() { - await Promise.resolve(this.loadProm); + await this.race.getCurrentProm(); // swap the data: the main column and the main row let temp = this.data.rowGroupTree; @@ -719,7 +722,7 @@ export class PivotModel extends Model { * @param {number[]} sortedColumn.groupId */ sortRows(sortedColumn) { - if (this.loadProm) { + if (this.race.getCurrentProm()) { return; // we are currently reloaded the table } @@ -745,7 +748,7 @@ export class PivotModel extends Model { // actually reload a lesser amount of information (but still, we need // to wait in case there is a pending load) metaData.activeMeasures.splice(index, 1); - await Promise.resolve(this.loadProm); + await Promise.resolve(this.race.getCurrentProm()); this.metaData = metaData; } else { metaData.activeMeasures.push(fieldName); @@ -1347,15 +1350,6 @@ export class PivotModel extends Model { * @param {Config} config */ async _loadData(config, prune = true) { - if (!this.loadProm) { - this.loadProm = new Promise((resolve) => { - this.resolveLoadProm = () => { - this.loadProm = null; - resolve(); - }; - }); - } - config.data = {}; // data will be completely recomputed const { data, metaData } = config; data.rowGroupTree = { root: { labels: [], values: [] }, directSubTrees: new Map() }; @@ -1372,33 +1366,25 @@ export class PivotModel extends Model { const rightDivisors = sections(metaData.fullColGroupBys); const divisors = cartesian(leftDivisors, rightDivisors); - this._subdivideGroup(group, divisors.slice(0, 1), config) - .then(() => { - return this._subdivideGroup(group, divisors.slice(1), config); - }) - .then(async () => { - // keep folded groups folded after the reload if the structure of the table is the same - if (prune && this._hasData(data) && this._hasData(this.data)) { - if ( - symmetricalDifference(metaData.rowGroupBys, this.metaData.rowGroupBys) - .length === 0 - ) { - this._pruneTree(data.rowGroupTree, this.data.rowGroupTree); - } - if ( - symmetricalDifference(metaData.colGroupBys, this.metaData.colGroupBys) - .length === 0 - ) { - this._pruneTree(data.colGroupTree, this.data.colGroupTree); - } - } + await this._subdivideGroup(group, divisors.slice(0, 1), config); + await this._subdivideGroup(group, divisors.slice(1), config); - this.data = config.data; - this.metaData = config.metaData; - this.resolveLoadProm(); - }); + // keep folded groups folded after the reload if the structure of the table is the same + if (prune && this._hasData(data) && this._hasData(this.data)) { + if ( + symmetricalDifference(metaData.rowGroupBys, this.metaData.rowGroupBys).length === 0 + ) { + this._pruneTree(data.rowGroupTree, this.data.rowGroupTree); + } + if ( + symmetricalDifference(metaData.colGroupBys, this.metaData.colGroupBys).length === 0 + ) { + this._pruneTree(data.colGroupTree, this.data.colGroupTree); + } + } - return this.loadProm; + this.data = config.data; + this.metaData = config.metaData; } /** * Extract the information in the read_group results (groupSubdivisions) diff --git a/addons/web/static/tests/views/graph_view_tests.js b/addons/web/static/tests/views/graph_view_tests.js index 619c99c4edd..0c853c97356 100644 --- a/addons/web/static/tests/views/graph_view_tests.js +++ b/addons/web/static/tests/views/graph_view_tests.js @@ -1,7 +1,7 @@ /** @odoo-module **/ import { BORDER_WHITE, DEFAULT_BG } from "@web/views/graph/colors"; -import { click, triggerEvent } from "@web/../tests/helpers/utils"; +import { click, makeDeferred, nextTick, triggerEvent } from "@web/../tests/helpers/utils"; import { createWebClient, doAction } from "@web/../tests/webclient/helpers"; import { dialogService } from "@web/core/dialog/dialog_service"; import { GraphArchParser } from "@web/views/graph/graph_arch_parser"; @@ -3308,4 +3308,118 @@ QUnit.module("Views", (hooks) => { ]); } ); + + QUnit.test( + "concurrent reloads: add a filter, and directly toggle a measure", + async function (assert) { + let def; + const graph = await makeView({ + serverData, + type: "graph", + resModel: "foo", + arch: ` + + + `, + searchViewArch: ` + + + `, + mockRPC: function (route, args) { + if (args.method === "web_read_group") { + return Promise.resolve(def); + } + }, + }); + + checkDatasets(assert, graph, ["data", "label"], { + data: [4, 4], + label: "Count", + }); + + // Set a domain (this reload is delayed) + def = makeDeferred(); + await toggleFilterMenu(graph); + await toggleMenuItem(graph, "My Filter"); + + checkDatasets(assert, graph, ["data", "label"], { + data: [4, 4], + label: "Count", + }); + + // Toggle a measure + await toggleMenu(graph, "Measures"); + await toggleMenuItem(graph, "Foo"); + + checkDatasets(assert, graph, ["data", "label"], { + data: [4, 4], + label: "Count", + }); + + def.resolve(); + await nextTick(); + + checkDatasets(assert, graph, ["data", "label"], { + data: [82, 157], + label: "Foo", + }); + } + ); + + QUnit.test("change graph mode while loading a filter", async function (assert) { + let def; + const graph = await makeView({ + serverData, + type: "graph", + resModel: "foo", + arch: ` + + + `, + searchViewArch: ` + + + `, + mockRPC: function (route, args) { + if (args.method === "web_read_group") { + return Promise.resolve(def); + } + }, + }); + + checkDatasets(assert, graph, ["data", "label"], { + data: [4, 4], + label: "Count", + }); + checkModeIs(assert, graph, "line"); + + // Set a domain (this reload is delayed) + def = makeDeferred(); + await toggleFilterMenu(graph); + await toggleMenuItem(graph, "My Filter"); + + checkDatasets(assert, graph, ["data", "label"], { + data: [4, 4], + label: "Count", + }); + checkModeIs(assert, graph, "line"); + + // Change graph mode + await selectMode(graph, "bar"); + + checkDatasets(assert, graph, ["data", "label"], { + data: [4, 4], + label: "Count", + }); + checkModeIs(assert, graph, "line"); + + def.resolve(); + await nextTick(); + + checkDatasets(assert, graph, ["data", "label"], { + data: [1], + label: "Count", + }); + checkModeIs(assert, graph, "bar"); + }); });