Catrope has uploaded a new change for review. https://gerrit.wikimedia.org/r/195361
Change subject: Make transaction processing exception-safe ...................................................................... Make transaction processing exception-safe Make modifier methods return a function that reverses the changes they made. Then if an exception occurs during or after applyModifications(), we can just call these functions in reverse order. If an exception occurs during synchronization, then the tree could be in any sort of crazy broken state, so we rebuild it from scratch. In the future, we should really dispense with the distinction between operations and modifications. The only reason we need a modifications layer right now is because operations as currently implemented are not safely reversible: trying to reverse an operation would also cause synchronizer actions to be queued, and in order to compute these actions, queries are made to a half-baked tree which will probably cause exceptions. In the tests, assert that the linear model and the tree are unchanged after an exception. Bug: T70892 Change-Id: Ic7d0088861cac9d1873f66633402e1b7eef2d645 --- M src/dm/ve.dm.Document.js M src/dm/ve.dm.TransactionProcessor.js M tests/dm/ve.dm.TransactionProcessor.test.js 3 files changed, 122 insertions(+), 25 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/VisualEditor/VisualEditor refs/changes/61/195361/1 diff --git a/src/dm/ve.dm.Document.js b/src/dm/ve.dm.Document.js index 52f0ba0..72639b2 100644 --- a/src/dm/ve.dm.Document.js +++ b/src/dm/ve.dm.Document.js @@ -853,6 +853,20 @@ }; /** + * Rebuild the entire node tree from linear model data. + */ +ve.dm.Document.prototype.rebuildTree = function () { + var documentNode = this.getDocumentNode(); + this.rebuildNodes( + documentNode, + 0, + documentNode.getChildren().length, + 0, + this.data.getLength() + ); +}; + +/** * Fix up data so it can safely be inserted into the document data at an offset. * * TODO: this function needs more work but it seems to work, mostly diff --git a/src/dm/ve.dm.TransactionProcessor.js b/src/dm/ve.dm.TransactionProcessor.js index 230d661..7ff0136 100644 --- a/src/dm/ve.dm.TransactionProcessor.js +++ b/src/dm/ve.dm.TransactionProcessor.js @@ -23,6 +23,7 @@ this.transaction = transaction; this.operations = transaction.getOperations(); this.modificationQueue = []; + this.rollbackQueue = []; this.synchronizer = new ve.dm.DocumentSynchronizer( doc, transaction ); // Linear model offset that we're currently at. Operations in the transaction are ordered, so // the cursor only ever moves forward. @@ -82,20 +83,45 @@ */ ve.dm.TransactionProcessor.prototype.process = function ( presynchronizeHandler ) { var op; + + // First process each operation to gather modifications in the modification queue. + // If an exception occurs during this stage, we don't need to do anything to recover, + // because no modifications were made yet. + this.operationIndex = 0; // This loop is factored this way to allow operations to be skipped over or executed // from within other operations - this.operationIndex = 0; while ( ( op = this.nextOperation() ) ) { this.executeOperation( op ); } - this.applyModifications(); - if ( presynchronizeHandler ) { - presynchronizeHandler(); - } - this.synchronizer.synchronize( this.transaction ); - // Mark the transaction as committed or rolled back, as appropriate + // Apply the queued modifications + try { + this.applyModifications(); + } catch ( e ) { + // Restore the linear model to its original state + this.rollbackModifications(); + // Rethrow the exception + throw e; + } + // Mark the transaction as committed this.transaction.markAsApplied(); + + // Synchronize the node tree for the modifications we just made + try { + if ( presynchronizeHandler ) { + presynchronizeHandler(); + } + this.synchronizer.synchronize( this.transaction ); + } catch ( e ) { + // Restore the linear model to its original state + this.rollbackModifications(); + // The synchronizer may have left the tree in some sort of weird half-baked state, + // so rebuild it from scratch + this.document.rebuildTree(); + // Rethrow the exception + throw e; + } + }; /** @@ -116,16 +142,29 @@ }; /** - * Apply all modifications queued through #queueModification. + * Apply all modifications queued through #queueModification, and add their rollback functions + * to this.rollbackQueue. */ ve.dm.TransactionProcessor.prototype.applyModifications = function () { - var i, len, modifications = this.modificationQueue; + var i, len, modifier, modifications = this.modificationQueue; this.modificationQueue = []; for ( i = 0, len = modifications.length; i < len; i++ ) { - ve.dm.TransactionProcessor.modifiers[modifications[i].type].apply( - this, - modifications[i].args || [] - ); + modifier = ve.dm.TransactionProcessor.modifiers[modifications[i].type]; + // Add to the beginning of rollbackQueue, because the most recent change needs to + // be undone first + this.rollbackQueue.unshift( modifier.apply( this, modifications[i].args || [] ) ); + } +}; + +/** + * Roll back all modifications that have been applied so far. This invokes the callbacks returned + * by the modifier functions. + */ +ve.dm.TransactionProcessor.prototype.rollbackModifications = function () { + var i, len, rollbacks = this.rollbackQueue; + this.rollbackQueue = []; + for ( i = 0, len = rollbacks.length; i < len; i++ ) { + rollbacks[i](); } }; @@ -210,8 +249,10 @@ /** * Modifier methods. * - * Each method executes a specific type of linear model modification. Methods are called in the - * context of a transaction processor, so they work similar to normal methods on the object. + * Each method executes a specific type of linear model modification and returns a function that + * undoes the modification, in case we need to recover the previous linear model state. + * Methods are called in the context of a transaction processor, so they work similar to normal + * methods on the object. * * @class ve.dm.TransactionProcessor.modifiers * @singleton @@ -223,11 +264,15 @@ * @param {number} offset Offset to remove/insert at * @param {number} remove Number of elements to remove * @param {Array} [insert] Elements to insert + * @return {Function} Function that undoes the modification */ ve.dm.TransactionProcessor.modifiers.splice = function ( type, offset, remove, insert ) { insert = insert || []; - var obj = type === 'metadata' ? this.document.metadata : this.document.data; - obj.batchSplice( offset, remove, insert ); + var data = type === 'metadata' ? this.document.metadata : this.document.data, + removed = data.batchSplice( offset, remove, insert ); + return function () { + data.batchSplice( offset, insert.length, removed ); + }; }; /** @@ -237,10 +282,15 @@ * @param {number} index Index in that offset's metadata array to remove/insert at * @param {number} remove Number of elements to remove * @param {Array} [insert] Elements to insert + * @return {Function} Function that undoes the modification */ ve.dm.TransactionProcessor.modifiers.spliceMetadataAtOffset = function ( offset, index, remove, insert ) { insert = insert || []; - this.document.metadata.spliceMetadataAtOffset( offset, index, remove, insert ); + var metadata = this.document.metadata, + removed = metadata.spliceMetadataAtOffset( offset, index, remove, insert ); + return function () { + metadata.spliceMetadataAtOffset( offset, index, insert.length, removed ); + }; }; /** @@ -248,9 +298,15 @@ * * @param {number} offset Offset in data array * @param {ve.dm.AnnotationSet} annotations New set of annotations; overwrites old set + * @return {Function} Function that undoes the modification */ ve.dm.TransactionProcessor.modifiers.annotateData = function ( offset, annotations ) { - this.document.data.setAnnotationsAtOffset( offset, annotations ); + var data = this.document.data, + oldAnnotations = data.getAnnotationsFromOffset( offset ); + data.setAnnotationsAtOffset( offset, annotations ); + return function () { + data.setAnnotationsAtOffset( offset, oldAnnotations ); + }; }; /** @@ -259,19 +315,32 @@ * @param {number} offset Offset to annotate at * @param {number} index Index in that offset's metadata array * @param {ve.dm.AnnotationSet} annotations New set of annotations; overwrites old set + * @return {Function} Function that undoes the modification */ ve.dm.TransactionProcessor.modifiers.annotateMetadata = function ( offset, index, annotations ) { - this.document.metadata.setAnnotationsAtOffsetAndIndex( offset, index, annotations ); + var metadata = this.document.metadata, + oldAnnotations = metadata.getAnnotationsFromOffsetAndIndex( offset, index ); + metadata.setAnnotationsAtOffsetAndIndex( offset, index, annotations ); + return function () { + metadata.setAnnotationsAtOffsetAndIndex( offset, index, oldAnnotations ); + }; }; /** * Set an attribute at a given offset. * @param {number} offset Offset in data array * @param {string} key Attribute name - * @param {Mixed} to New attribute value + * @param {Mixed} value New attribute value + * @return {Function} Function that undoes the modification */ -ve.dm.TransactionProcessor.modifiers.setAttribute = function ( offset, key, to ) { - this.document.data.setAttributeAtOffset( offset, key, to ); +ve.dm.TransactionProcessor.modifiers.setAttribute = function ( offset, key, value ) { + var data = this.document.data, + item = data.getData( offset ), + oldValue = item.attributes && item.attributes[key]; + data.setAttributeAtOffset( offset, key, value ); + return function () { + data.setAttributeAtOffset( offset, key, oldValue ); + }; }; /** diff --git a/tests/dm/ve.dm.TransactionProcessor.test.js b/tests/dm/ve.dm.TransactionProcessor.test.js index 962e91f..7d4f5b3 100644 --- a/tests/dm/ve.dm.TransactionProcessor.test.js +++ b/tests/dm/ve.dm.TransactionProcessor.test.js @@ -292,6 +292,14 @@ data[4].type = '/paragraph'; } }, + 'conversion with wrong closing': { + calls: [ + ['pushReplace', 0, 1, [{ type: 'paragraph' }]], + ['pushRetain', 3], + ['pushReplace', 4, 1, [{ type: '/paragraph' }, { type: 'paragraph' }]] + ], + exception: /Unbalanced set of replace operations found/ + }, 'splitting an element': { calls: [ ['pushRetain', 2], @@ -659,7 +667,7 @@ }; for ( msg in cases ) { - n += ( 'expected' in cases[msg] ) ? 4 : 1; + n += ( 'expected' in cases[msg] ) ? 4 : 3; } QUnit.expect( n ); @@ -721,7 +729,13 @@ testDoc.commit( tx ); }, cases[msg].exception, - 'commit: ' + msg + 'exception thrown: ' + msg + ); + assert.deepEqualWithDomElements( testDoc.getFullData(), originalDoc.getFullData(), 'data unmodified: ' + msg ); + assert.equalNodeTree( + testDoc.getDocumentNode(), + originalDoc.getDocumentNode(), + 'tree unmodified: ' + msg ); } } -- To view, visit https://gerrit.wikimedia.org/r/195361 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ic7d0088861cac9d1873f66633402e1b7eef2d645 Gerrit-PatchSet: 1 Gerrit-Project: VisualEditor/VisualEditor Gerrit-Branch: master Gerrit-Owner: Catrope <roan.katt...@gmail.com> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits