From 7af09db878997d1e5af09e4a3c314e6df5311e55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexandre=20K=C3=BChn?= Date: Wed, 4 Jul 2018 17:17:29 +0200 Subject: [FIX] web: no concurrent reload on basic model Before this commit, there was a traceback in the following scenario: Let's have a kanban view with groupby and a domain. If we clear the domain then clear the groupby, there could be a crash. This crash occurs because when we clear the groupby, it performs a `reload`, and assumes that the datapoint contains grouped data. However, if this computation takes some time, it is possible to overwrite the datapoint so that there is no longer grouped data. This commit fixes the issue by enforcing at most 1 execution of the method `reload` active at any time. Consequently, successive reloads operations are now much slower. Note that it uses the single non-reentrant mutex on the basic_model to enforce this rule, which means: - No more concurrent `reload` and `save`. - If an execution having the mutex in `reload` come accross the same mutex, it is blocked forever. This is hopefully not the case at the moment, however a reentrant mutex should be considered in the future. --- .../web/static/src/js/views/basic/basic_model.js | 132 ++++++++++++--------- 1 file changed, 73 insertions(+), 59 deletions(-) (limited to 'addons/web/static/src/js/views/basic') diff --git a/addons/web/static/src/js/views/basic/basic_model.js b/addons/web/static/src/js/views/basic/basic_model.js index 82bb9e586a8..32fb23d983a 100644 --- a/addons/web/static/src/js/views/basic/basic_model.js +++ b/addons/web/static/src/js/views/basic/basic_model.js @@ -814,7 +814,8 @@ var BasicModel = AbstractModel.extend({ return this.mutex.exec(this._applyChange.bind(this, record_id, changes, options)); }, /** - * Reload all data for a given resource + * Reload all data for a given resource. At any time there is at most one + * reload operation active. * * @param {string} id local id for a resource * @param {Object} [options] @@ -823,64 +824,7 @@ var BasicModel = AbstractModel.extend({ * @returns {Deferred} resolves to the id of the resource */ reload: function (id, options) { - options = options || {}; - var element = this.localData[id]; - - if (element.type === 'record') { - if (!options.currentId && (('currentId' in options) || this.isNew(id))) { - var params = { - context: element.context, - fieldsInfo: element.fieldsInfo, - fields: element.fields, - viewType: element.viewType, - }; - return this._makeDefaultRecord(element.model, params); - } - if (!options.keepChanges) { - this.discardChanges(id, {rollback: false}); - } - } else if (element._changes) { - delete element.tempLimitIncrement; - _.each(element._changes, function (change) { - delete change.isNew; - }); - } - - if (options.context !== undefined) { - element.context = options.context; - } - if (options.domain !== undefined) { - element.domain = options.domain; - } - if (options.groupBy !== undefined) { - element.groupedBy = options.groupBy; - } - if (options.limit !== undefined) { - element.limit = options.limit; - } - if (options.offset !== undefined) { - this._setOffset(element.id, options.offset); - } - if (options.loadMoreOffset !== undefined) { - element.loadMoreOffset = options.loadMoreOffset; - } else { - // reset if not specified - element.loadMoreOffset = 0; - } - if (options.currentId !== undefined) { - element.res_id = options.currentId; - } - if (options.ids !== undefined) { - element.res_ids = options.ids; - element.count = element.res_ids.length; - } - if (element.type === 'record') { - element.offset = _.indexOf(element.res_ids, element.res_id); - } - var loadOptions = _.pick(options, 'fieldNames', 'viewType'); - return this._load(element, loadOptions).then(function (result) { - return result.id; - }); + return this.mutex.exec(this._reload.bind(this, id, options)); }, /** * In some case, we may need to remove an element from a list, without going @@ -4008,6 +3952,76 @@ var BasicModel = AbstractModel.extend({ }); }); }, + /** + * Reload all data for a given resource + * + * @private + * @param {string} id local id for a resource + * @param {Object} [options] + * @param {boolean} [options.keepChanges=false] if true, doesn't discard the + * changes on the record before reloading it + * @returns {Deferred} resolves to the id of the resource + */ + _reload: function (id, options) { + options = options || {}; + var element = this.localData[id]; + + if (element.type === 'record') { + if (!options.currentId && (('currentId' in options) || this.isNew(id))) { + var params = { + context: element.context, + fieldsInfo: element.fieldsInfo, + fields: element.fields, + viewType: element.viewType, + }; + return this._makeDefaultRecord(element.model, params); + } + if (!options.keepChanges) { + this.discardChanges(id, {rollback: false}); + } + } else if (element._changes) { + delete element.tempLimitIncrement; + _.each(element._changes, function (change) { + delete change.isNew; + }); + } + + if (options.context !== undefined) { + element.context = options.context; + } + if (options.domain !== undefined) { + element.domain = options.domain; + } + if (options.groupBy !== undefined) { + element.groupedBy = options.groupBy; + } + if (options.limit !== undefined) { + element.limit = options.limit; + } + if (options.offset !== undefined) { + this._setOffset(element.id, options.offset); + } + if (options.loadMoreOffset !== undefined) { + element.loadMoreOffset = options.loadMoreOffset; + } else { + // reset if not specified + element.loadMoreOffset = 0; + } + if (options.currentId !== undefined) { + element.res_id = options.currentId; + } + if (options.ids !== undefined) { + element.res_ids = options.ids; + element.count = element.res_ids.length; + } + if (element.type === 'record') { + element.offset = _.indexOf(element.res_ids, element.res_id); + } + var loadOptions = _.pick(options, 'fieldNames', 'viewType'); + return this._load(element, loadOptions).then(function (result) { + return result.id; + }); + }, /** * Allows to save a value in the specialData cache associated to a given * record and fieldName. If the value in the cache was already the given -- cgit v1.2.3 From 7764db7a34f05b55b56c2301f9c0b2a0098c2939 Mon Sep 17 00:00:00 2001 From: len-odoo Date: Wed, 27 Jun 2018 13:54:36 +0200 Subject: [FIX] web: handle onchange on a on2many with an embedded one2many on multiple pages Suppose there is a one2many field embedded in a one2many, (say we are viewing record A with one2many field B itself with a one2many C). Furthermore, there is a second page of the one2many field B. If an onchange is applied on B, then the server replies with a 1 command for all B records attached to the currently viewed object A. This includes a list of commands [[5], [4, C_id]*] for each B record, in particular B records that are on the second page. Since they are on the second page, they might not have been fetched by the frontend; in that case the corresponding C_ids are thus unknown. As a consequence, on save the frontend sends a command 0 to create the records for all unknown C_ids. Since it doesn't have any value to give to the 0 command, the row creation usually crash in the backend. Another wrong behaviour is fixed at the same time: if the server sends an update on the list of C ids with 4 commands. In that case, if the record has not been prefetched, we have no way to know if the list changed or not. To solve this, when we receive the 4 commands by the server, we send back 4 commands. coauthored by @aab-odoo opw 1835936 --- addons/web/static/src/js/views/basic/basic_model.js | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) (limited to 'addons/web/static/src/js/views/basic') diff --git a/addons/web/static/src/js/views/basic/basic_model.js b/addons/web/static/src/js/views/basic/basic_model.js index 32fb23d983a..7621fe35446 100644 --- a/addons/web/static/src/js/views/basic/basic_model.js +++ b/addons/web/static/src/js/views/basic/basic_model.js @@ -2860,13 +2860,10 @@ var BasicModel = AbstractModel.extend({ continue; } changes = this._generateChanges(relRecord, options); - if (changes.id) { + if (!this.isNew(relRecord.id)) { // the subrecord already exists in db + commands[fieldName].push(x2ManyCommands.link_to(relRecord.res_id)); delete changes.id; - if (this.isNew(record.id)) { - // if the main record is new, link the subrecord to it - commands[fieldName].push(x2ManyCommands.link_to(relRecord.res_id)); - } if (!_.isEmpty(changes)) { commands[fieldName].push(x2ManyCommands.update(relRecord.res_id, changes)); } -- cgit v1.2.3