Use generator.return rather than throw for cancellation

mobxjs · Mar 12, 2018 · 9e805b5 · 9e805b5
1 parent 9984b12
commit 9e805b5
Show file tree

Hide file tree

Showing 2 changed files with 74 additions and 65 deletions.
diff --git a/src/api/flow.ts b/src/api/flow.ts
@@ -117,53 +117,58 @@ export function createFlowGenerator(name: string, generator: Function) {
         const args = arguments
         const runId = ++generatorId
         const gen = action(`${name} - runid: ${runId} - init`, generator).apply(ctx, args)
-        let stepId = 0
-        let resolver: (value: any) => void
         let rejector: (error: any) => void
 
-        function onFulfilled(res: any) {
-            let ret
-            try {
-                ret = action(`${name} - runid: ${runId} - yield ${stepId++}`, gen.next).call(
-                    gen,
-                    res
-                )
-            } catch (e) {
-                return rejector(e)
+        const res = new Promise(function(resolve, reject) {
+            let stepId = 0
+            rejector = reject
+
+            function onFulfilled(res: any) {
+                let ret
+                try {
+                    ret = action(`${name} - runid: ${runId} - yield ${stepId++}`, gen.next).call(
+                        gen,
+                        res
+                    )
+                } catch (e) {
+                    return reject(e)
+                }
+                next(ret)
+                return null
             }
-            next(ret)
-            return null
-        }
 
-        function onRejected(err: any) {
-            let ret
-            try {
-                ret = action(`${name} - runid: ${runId} - yield ${stepId++}`, gen.throw).call(
-                    gen,
-                    err
-                )
-            } catch (e) {
-                return rejector(e)
+            function onRejected(err: any) {
+                let ret
+                try {
+                    ret = action(`${name} - runid: ${runId} - yield ${stepId++}`, gen.throw).call(
+                        gen,
+                        err
+                    )
+                } catch (e) {
+                    return reject(e)
+                }
+                next(ret)
             }
-            next(ret)
-        }
 
-        function next(ret: any) {
-            if (ret.done) return resolver(ret.value)
-            // TODO: support more type of values? See https://github.com/tj/co/blob/249bbdc72da24ae44076afd716349d2089b31c4c/index.js#L100
-            if (!ret.value || typeof ret.value.then !== "function")
-                return fail("Only promises can be yielded to asyncAction, got: " + ret)
-            return ret.value.then(onFulfilled, onRejected)
-        }
+            function next(ret: any) {
+                if (ret.done) return resolve(ret.value)
+                // TODO: support more type of values? See https://github.com/tj/co/blob/249bbdc72da24ae44076afd716349d2089b31c4c/index.js#L100
+                if (!ret.value || typeof ret.value.then !== "function")
+                    return fail("Only promises can be yielded to asyncAction, got: " + ret)
+                return ret.value.then(onFulfilled, onRejected)
+            }
 
-        const res = new Promise(function(resolve, reject) {
-            resolver = resolve
-            rejector = reject
             onFulfilled(undefined) // kick off the process
         }) as any
-        res.cancel = function() {
-            onRejected(new Error("FLOW_CANCELLED"))
-        }
+
+        res.cancel = action(`${name} - runid: ${runId} - cancel`, function() {
+            try {
+                gen.return()
+                rejector(new Error("FLOW_CANCELLED"))
+            } catch (e) {
+                rejector(e) // there could be a throwing finally block
+            }
+        })
         return res
     }
 }

diff --git a/test/base/flow.js b/test/base/flow.js
@@ -170,62 +170,72 @@ test("flows can be cancelled - 1 - uncatched cancellation", done => {
 
     const promise = start()
     promise.then(
-        () => fail(),
+        () => {
+            debugger
+            fail()
+        },
         err => {
+            debugger
             expect(steps).toBe(1)
             expect("" + err).toBe("Error: FLOW_CANCELLED")
             done()
         }
     )
+    debugger
     promise.cancel()
 })
 
-test("flows can be cancelled - 2 - catch cancellation in generator", done => {
+test("flows can be cancelled - 2 - finally clauses are run", done => {
     let steps = 0
+    let finallyHandled = false
     const start = flow(function*() {
         steps = 1
         try {
             yield Promise.resolve()
             steps = 2
-        } catch (e) {
+        } finally {
             expect(steps).toBe(1)
-            expect(e.toString()).toBe("Error: FLOW_CANCELLED")
-            return 4
+            finallyHandled = true
         }
     })
     const promise = start()
     promise.then(
         res => {
-            expect(res).toBe(4)
-            done()
+            fail()
         },
         err => {
-            fail()
+            expect("" + err).toBe("Error: FLOW_CANCELLED")
+            expect(finallyHandled).toBeTruthy()
+            done()
         }
     )
     promise.cancel()
 })
 
-test("flows can be cancelled - 3 - rethrow cancellation", done => {
-    let steps = 0
+test("flows can be cancelled - 3 - throw in finally should be catched", done => {
+    const counter = mobx.observable({ counter: 0 })
+    const d = mobx.reaction(() => counter.counter, () => {})
+    mobx.configure({ enforceActions: true })
+
     const start = flow(function*() {
-        steps = 1
+        counter.counter = 1
         try {
             yield Promise.resolve()
-            steps = 2
-        } catch (e) {
-            expect(steps).toBe(1)
-            expect(e.toString()).toBe("Error: FLOW_CANCELLED")
-            throw e // rethrow
+            counter.counter = 15
+        } finally {
+            counter.counter = 4
+            throw "OOPS"
         }
     })
 
     const promise = start()
     promise.then(
-        () => fail(),
+        () => fail("flow should not have failed"),
         err => {
-            expect(steps).toBe(1)
-            expect("" + err).toBe("Error: FLOW_CANCELLED")
+            expect("" + err).toBe("OOPS")
+            expect(counter.counter).toBe(4)
+            mobx.configure({ enforceActions: false })
+            d()
             done()
         }
     )
@@ -236,14 +246,8 @@ test("flows can be cancelled - 4 - pending Promise will be ignored", done => {
     let steps = 0
     const start = flow(function*() {
         steps = 1
-        try {
-            yield Promise.reject("This won't be catched anywhere!") // cancel will resolve this flow before this one is throw, so this promise goes uncatched
-            steps = 2
-        } catch (e) {
-            expect(steps).toBe(1)
-            expect(e.toString()).toBe("Error: FLOW_CANCELLED")
-            throw e
-        }
+        yield Promise.reject("This won't be catched anywhere!") // cancel will resolve this flow before this one is throw, so this promise goes uncatched
+        steps = 2
     })
 
     const promise = start()