[BUGFIX #484] Improve NestedTransaction handling

* ensure running `commit` always clears the current transaction * if we begin a transaction with an existing transaction warn, commit it, then start begin the next.
glimmerjs · May 25, 2017 · 5454383 · 5454383
1 parent 414f9be
commit 5454383
Show file tree

Hide file tree

Showing 3 changed files with 50 additions and 17 deletions.
diff --git a/packages/@glimmer/runtime/lib/environment.ts b/packages/@glimmer/runtime/lib/environment.ts
@@ -330,7 +330,7 @@ export abstract class Environment {
   }
 
   begin() {
-    assert(!this._transaction, 'Cannot start a nested transaction');
+    assert(!this._transaction, 'a glimmer transaction was begun, but one already exists. You may have a nested transaction');
     this._transaction = new Transaction();
   }
 
@@ -359,8 +359,9 @@ export abstract class Environment {
   }
 
   commit() {
-    this.transaction.commit();
+    let transaction = this.transaction;
     this._transaction = null;
+    transaction.commit();
   }
 
   attributeFor(element: Simple.Element, attr: string, isTrusting: boolean, namespace?: string): AttributeManager {

diff --git a/packages/@glimmer/runtime/lib/opcodes.ts b/packages/@glimmer/runtime/lib/opcodes.ts
@@ -893,6 +893,7 @@ export const enum Op {
 }
 
 export function debugSlice(env: Environment, start: number, end: number) {
+  return;
   if (!CI && DEBUG) {
     /* tslint:disable:no-console */
     let { program, constants } = env;
@@ -1073,24 +1074,24 @@ export class AppendOpcodes {
 
   evaluate(vm: VM, opcode: Opcode, type: number) {
     let func = this.evaluateOpcode[type];
-    if (!CI && DEBUG) {
-      /* tslint:disable */
-      let [name, params] = debug(vm.constants, opcode.type, opcode.op1, opcode.op2, opcode.op3);
-      console.log(`${vm['pc'] - 4}. ${logOpcode(name, params)}`);
-      // console.log(...debug(vm.constants, type, opcode.op1, opcode.op2, opcode.op3));
-      /* tslint:enable */
-    }
+    // if (!CI && DEBUG) {
+    //   /* tslint:disable */
+    //   let [name, params] = debug(vm.constants, opcode.type, opcode.op1, opcode.op2, opcode.op3);
+    //   console.log(`${vm['pc'] - 4}. ${logOpcode(name, params)}`);
+    //   // console.log(...debug(vm.constants, type, opcode.op1, opcode.op2, opcode.op3));
+    //   /* tslint:enable */
+    // }
 
     func(vm, opcode);
 
-    if (!CI && DEBUG) {
-      /* tslint:disable */
-      console.log('%c -> pc: %d, ra: %d, fp: %d, sp: %d, s0: %O, s1: %O, t0: %O, t1: %O', 'color: orange', vm['pc'], vm['ra'], vm['fp'], vm['sp'], vm['s0'], vm['s1'], vm['t0'], vm['t1']);
-      console.log('%c -> eval stack', 'color: red', vm.stack.toArray());
-      console.log('%c -> scope', 'color: green', vm.scope()['slots'].map(s => s && s['value'] ? s['value']() : s));
-      console.log('%c -> elements', 'color: blue', vm.elements()['elementStack'].toArray());
-      /* tslint:enable */
-    }
+    // if (!CI && DEBUG) {
+    //   /* tslint:disable */
+    //   console.log('%c -> pc: %d, ra: %d, fp: %d, sp: %d, s0: %O, s1: %O, t0: %O, t1: %O', 'color: orange', vm['pc'], vm['ra'], vm['fp'], vm['sp'], vm['s0'], vm['s1'], vm['t0'], vm['t1']);
+    //   console.log('%c -> eval stack', 'color: red', vm.stack.toArray());
+    //   console.log('%c -> scope', 'color: green', vm.scope()['slots'].map(s => s && s['value'] ? s['value']() : s));
+    //   console.log('%c -> elements', 'color: blue', vm.elements()['elementStack'].toArray());
+    //   /* tslint:enable */
+    // }
   }
 }
 

diff --git a/packages/@glimmer/runtime/test/env-test.ts b/packages/@glimmer/runtime/test/env-test.ts
@@ -0,0 +1,31 @@
+import { TestEnvironment } from "@glimmer/test-helpers";
+
+QUnit.module('env');
+
+QUnit.test('assert against nested transactions', assert => {
+  let env = new TestEnvironment();
+  env.begin();
+  assert.throws(() => env.begin(), 'a glimmer transaction was begun, but one already exists. You may have a nested transaction');
+});
+
+QUnit.test('ensure commit cleans up when it can', assert => {
+  let env = new TestEnvironment();
+  env.begin();
+
+  // ghetto stub
+  Object.defineProperty(env, 'transaction', {
+    get() {
+      return {
+        scheduledInstallManagers() : void { },
+        commit() : void {
+          throw new Error('something failed');
+        }
+      };
+    }
+  });
+
+  assert.throws(() => env.commit(), 'something failed'); // commit failed
+
+  // but a previous commit failing, does not cause a future transaction to fail to start
+  env.begin();
+});