pengyuthon/odoo_source
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

[FIX] web: no concurrent reload on basic model

Browse Source 
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.
This commit is contained in:
Alexandre Kühn
2018-07-05 14:04:33 +02:00
parent b27d009ab4
commit 7af09db878
2 changed files with 123 additions and 59 deletions
Download Patch File Download Diff File Expand all files Collapse all files
addons/web/static/src/js/views/basic/basic_model.js
+73 -59
View File
@@ -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<string>} 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<string>} 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
addons/web/static/tests/views/kanban_tests.js
+50
View File
@@ -2840,6 +2840,56 @@ QUnit.module('Views', {
testUtils.unpatch(mixins.ParentedMixin);
});
QUnit.test('grouped kanban becomes ungrouped when clearing domain then clearing groupby', function (assert) {
// in this test, we simulate that clearing the domain is slow, so that
// clearing the groupby does not corrupt the data handled while
// reloading the kanban view.
assert.expect(4);
var def = $.Deferred();
var kanban = createView({
View: KanbanView,
model: 'partner',
data: this.data,
arch: '<kanban class="o_kanban_test">' +
'<field name="bar"/>' +
'<templates><t t-name="kanban-box">' +
'<div><field name="foo"/></div>' +
'</t></templates></kanban>',
domain: [['foo', '=', 'norecord']],
groupBy: ['bar'],
mockRPC: function (route, args) {
var result = this._super(route, args);
if (args.method === 'read_group') {
var isFirstUpdate = _.isEmpty(args.kwargs.domain) &&
args.kwargs.groupby &&
args.kwargs.groupby[0] === 'bar';
if (isFirstUpdate) {
return def.then(_.constant(result));
}
}
return result;
},
});
assert.ok(kanban.$('.o_kanban_view').hasClass('o_kanban_grouped'),
"the kanban view should be grouped");
assert.notOk(kanban.$('.o_kanban_view').hasClass('o_kanban_ungrouped'),
"the kanban view should not be ungrouped");
kanban.update({domain: []}); // 1st update on kanban view
kanban.update({groupBy: false}); // 2n update on kanban view
def.resolve(); // simulate slow 1st update of kanban view
assert.notOk(kanban.$('.o_kanban_view').hasClass('o_kanban_grouped'),
"the kanban view should not longer be grouped");
assert.ok(kanban.$('.o_kanban_view').hasClass('o_kanban_ungrouped'),
"the kanban view should have become ungrouped");
kanban.destroy();
});
});
});