(core) Fix race condition in bundling actions for undo, when actions are submitted close together.

Summary:
The way linkId was set on actions to tie them together for undo bundling was
incorrect. This diff fixes it by moves the setting of linkIds to Sharing.ts,
which already serializes the processing of actions.

Test Plan: Added a test case for submitting actions together while bundling (which fails without this change).

Reviewers: paulfitz

Reviewed By: paulfitz

Differential Revision: https://phab.getgrist.com/D2716
This commit is contained in:
Dmitry S 2021-01-27 18:03:30 -05:00
parent 14cdd47675
commit 9fa5d4c9d6
2 changed files with 12 additions and 13 deletions

View File

@ -14,6 +14,7 @@ import flatten = require('lodash/flatten');
import remove = require('lodash/remove'); import remove = require('lodash/remove');
import zipObject = require('lodash/zipObject'); import zipObject = require('lodash/zipObject');
import * as moment from 'moment-timezone'; import * as moment from 'moment-timezone';
import fetch from 'node-fetch';
import * as tmp from 'tmp'; import * as tmp from 'tmp';
import {getEnvContent, LocalActionBundle} from 'app/common/ActionBundle'; import {getEnvContent, LocalActionBundle} from 'app/common/ActionBundle';
@ -60,7 +61,6 @@ import {expandQuery} from './ExpandedQuery';
import {GranularAccess} from './GranularAccess'; import {GranularAccess} from './GranularAccess';
import {OnDemandActions} from './OnDemandActions'; import {OnDemandActions} from './OnDemandActions';
import {findOrAddAllEnvelope, Sharing} from './Sharing'; import {findOrAddAllEnvelope, Sharing} from './Sharing';
import fetch from 'node-fetch';
bluebird.promisifyAll(tmp); bluebird.promisifyAll(tmp);
@ -732,11 +732,8 @@ export class ActiveDoc extends EventEmitter {
// Be careful not to sneak into user action queue before Calculate action, otherwise // Be careful not to sneak into user action queue before Calculate action, otherwise
// there'll be a deadlock. // there'll be a deadlock.
await this.waitForInitialization(); await this.waitForInitialization();
const newOptions = {linkId: docSession.linkId, ...options};
// Granular access control implemented in _applyUserActions. // Granular access control implemented in _applyUserActions.
const result: ApplyUAResult = await this._applyUserActions(docSession, actions, newOptions); return await this._applyUserActions(docSession, actions, options);
docSession.linkId = docSession.shouldBundleActions ? result.actionNum : 0;
return result;
} }
/** /**
@ -913,12 +910,13 @@ export class ActiveDoc extends EventEmitter {
const permitKey = await permitStore.setPermit({docId: forkIds.docId, const permitKey = await permitStore.setPermit({docId: forkIds.docId,
otherDocId: this.docName}); otherDocId: this.docName});
try { try {
const url = await this._docManager.gristServer.getHomeUrlByDocId(forkIds.docId, `/api/docs/${forkIds.docId}/create-fork`); const url = await this._docManager.gristServer.getHomeUrlByDocId(
forkIds.docId, `/api/docs/${forkIds.docId}/create-fork`);
const resp = await fetch(url, { const resp = await fetch(url, {
method: 'POST', method: 'POST',
body: JSON.stringify({ srcDocId: this.docName }), body: JSON.stringify({ srcDocId: this.docName }),
headers: { headers: {
Permit: permitKey, 'Permit': permitKey,
'Content-Type': 'application/json', 'Content-Type': 'application/json',
}, },
}); });

View File

@ -209,6 +209,10 @@ export class Sharing {
branch: Branch, docSession: OptDocSession|null): Promise<UserResult> { branch: Branch, docSession: OptDocSession|null): Promise<UserResult> {
const client = docSession && docSession.client; const client = docSession && docSession.client;
if (docSession?.linkId) {
info.linkId = docSession.linkId;
}
const {sandboxActionBundle, undo, docActions} = const {sandboxActionBundle, undo, docActions} =
await this._modificationLock.runExclusive(() => this._applyActionsToDataEngine(docSession, userActions)); await this._modificationLock.runExclusive(() => this._applyActionsToDataEngine(docSession, userActions));
@ -271,12 +275,6 @@ export class Sharing {
} }
await this._activeDoc.processActionBundle(ownActionBundle); await this._activeDoc.processActionBundle(ownActionBundle);
// In the future, we'll save (and share) the result of applying one bundle of UserActions
// as a single ActionBundle with one actionNum. But the old ActionLog saves on UserAction
// per actionNum, using linkId to "bundle" them for the purpose of undo-redo. We simulate
// it here by breaking up ActionBundle into as many old-style ActionGroups as there are
// UserActions, and associating all DocActions with the first of these ActionGroups.
// Broadcast the action to connected browsers. // Broadcast the action to connected browsers.
const actionGroup = asActionGroup(this._actionHistory, localActionBundle, { const actionGroup = asActionGroup(this._actionHistory, localActionBundle, {
client, client,
@ -295,6 +293,9 @@ export class Sharing {
} finally { } finally {
await this._activeDoc.finishedActions(); await this._activeDoc.finishedActions();
} }
if (docSession) {
docSession.linkId = docSession.shouldBundleActions ? localActionBundle.actionNum : 0;
}
return { return {
actionNum: localActionBundle.actionNum, actionNum: localActionBundle.actionNum,
retValues: sandboxActionBundle.retValues, retValues: sandboxActionBundle.retValues,