From 5e9109fa4f2e9b16562ea2afac8ebcddb8f50324 Mon Sep 17 00:00:00 2001
From: Brian Kim <briankimpossible@gmail.com>
Date: Tue, 28 Apr 2020 23:56:50 -0400
Subject: [PATCH] stop deadlocks when errors are thrown upwards

---
 src/__tests__/errors.tsx | 144 +++++++++++++++++++
 src/__tests__/index.tsx  | 295 +--------------------------------------
 src/index.ts             |  23 ++-
 3 files changed, 164 insertions(+), 298 deletions(-)
 create mode 100644 src/__tests__/errors.tsx

diff --git a/src/__tests__/errors.tsx b/src/__tests__/errors.tsx
new file mode 100644
index 000000000..418b188ce
--- /dev/null
+++ b/src/__tests__/errors.tsx
@@ -0,0 +1,144 @@
+/** @jsx createElement */
+import {createElement, Child, Context} from "../index";
+import {renderer} from "../dom";
+
+describe("errors", () => {
+	afterEach(async () => {
+		document.body.innerHTML = "";
+		await renderer.render(null, document.body);
+	});
+
+	test("sync function throws", () => {
+		const error = new Error("sync function throws");
+		function Thrower(): never {
+			throw error;
+		}
+
+		expect(() => renderer.render(<Thrower />, document.body)).toThrow(error);
+	});
+
+	test("async function throws", async () => {
+		const error = new Error("async function throws");
+
+		async function Thrower(): Promise<never> {
+			throw error;
+		}
+
+		await expect(renderer.render(<Thrower />, document.body)).rejects.toBe(
+			error,
+		);
+	});
+
+	test("sync generator throws", () => {
+		const error = new Error("sync generator throws");
+		function* Thrower() {
+			yield 1;
+			yield 2;
+			throw error;
+		}
+
+		renderer.render(<Thrower />, document.body);
+		expect(document.body.innerHTML).toEqual("1");
+		renderer.render(<Thrower />, document.body);
+		expect(document.body.innerHTML).toEqual("2");
+		expect(() => renderer.render(<Thrower />, document.body)).toThrow(error);
+	});
+
+	test("async generator throws", async () => {
+		const error = new Error("async generator throws");
+		async function* Thrower(this: Context) {
+			let i = 1;
+			for await (const _ of this) {
+				if (i > 3) {
+					throw error;
+				}
+				yield i++;
+			}
+		}
+
+		await renderer.render(<Thrower />, document.body);
+		await renderer.render(<Thrower />, document.body);
+		await renderer.render(<Thrower />, document.body);
+		await expect(renderer.render(<Thrower />, document.body)).rejects.toBe(
+			error,
+		);
+	});
+
+	// TODO: figure out how to catch the promise rejection
+	test.skip("async generator throws independently", async () => {
+		const error = new Error("async generator throws independently");
+		async function* Thrower(this: Context) {
+			yield 1;
+			yield 2;
+			yield 3;
+			throw error;
+		}
+
+		renderer.render(<Thrower />, document.body);
+		await new Promise(() => {});
+	});
+
+	// TODO: figure out why this rejects
+	test.skip("async generator throws in async generator", async () => {
+		const error = new Error("async generator throws in async generator");
+		/* eslint-disable require-yield */
+		async function* Thrower() {
+			throw error;
+		}
+		/* eslint-enable require-yield */
+
+		async function* Component(this: Context) {
+			for await (const _ of this) {
+				yield <Thrower />;
+			}
+		}
+
+		await expect(renderer.render(<Component />, document.body)).rejects.toBe(
+			error,
+		);
+
+		await new Promise(() => {});
+	});
+
+	test("sync function throws, sync generator catches", () => {
+		function Thrower(): Promise<never> {
+			throw new Error("sync function throws, sync generator catches");
+		}
+
+		function* Component(): Generator<Child> {
+			try {
+				yield <Thrower />;
+			} catch (err) {
+				return <span>Error</span>;
+			}
+		}
+
+		renderer.render(<Component />, document.body);
+		expect(document.body.innerHTML).toEqual("<span>Error</span>");
+	});
+
+	test("async function throws, sync generator catches", async () => {
+		async function Thrower(): Promise<never> {
+			throw new Error("async function throws, sync generator catches");
+		}
+
+		function* Component(): Generator<Child> {
+			try {
+				yield <Thrower />;
+			} catch (err) {
+				return <span>Error</span>;
+			}
+		}
+
+		await renderer.render(
+			<div>
+				<Component />
+			</div>,
+			document.body,
+		);
+
+		expect(document.body.innerHTML).toEqual("<div><span>Error</span></div>");
+		await new Promise((resolve) => setTimeout(resolve, 20));
+		expect(document.body.innerHTML).toEqual("<div><span>Error</span></div>");
+	});
+});
diff --git a/src/__tests__/index.tsx b/src/__tests__/index.tsx
index 21c276e7f..9a9bcc0bc 100644
--- a/src/__tests__/index.tsx
+++ b/src/__tests__/index.tsx
@@ -12,7 +12,6 @@ import {renderer} from "../dom";
 
 // TODO: start splitting out these tests into separate files
 
-// TODO: no-unreachable is throwing false positives in some tests
 /* eslint-disable no-unreachable */
 describe("sync function component", () => {
 	afterEach(async () => {
@@ -830,299 +829,6 @@ describe("sync generator component", () => {
 		expect(mock).toHaveBeenCalledTimes(1);
 	});
 
-	test("errors are caught", () => {
-		function Thrower(): never {
-			throw new Error("errors are caught");
-		}
-
-		function* Child(): Generator<number> {
-			yield 1;
-			yield 2;
-			yield <Thrower />;
-		}
-
-		function* Component(): Generator<Child> {
-			try {
-				while (true) {
-					yield (
-						<span>
-							<Child />
-						</span>
-					);
-				}
-			} catch (err) {
-				return <span>Error: {err.message}</span>;
-			}
-		}
-
-		renderer.render(
-			<div>
-				<Component />
-			</div>,
-			document.body,
-		);
-		expect(document.body.innerHTML).toEqual("<div><span>1</span></div>");
-		renderer.render(
-			<div>
-				<Component />
-			</div>,
-			document.body,
-		);
-		expect(document.body.innerHTML).toEqual("<div><span>2</span></div>");
-		renderer.render(
-			<div>
-				<Component />
-			</div>,
-			document.body,
-		);
-		expect(document.body.innerHTML).toEqual(
-			"<div><span>Error: errors are caught</span></div>",
-		);
-	});
-
-	test("errors caught and rerendered", () => {
-		function Thrower(): never {
-			throw new Error("errors caught and rerendered");
-		}
-
-		function* Child(): Generator<number> {
-			yield 1;
-			yield 2;
-			yield <Thrower />;
-		}
-
-		function* Component(): Generator<Child> {
-			try {
-				while (true) {
-					yield (
-						<span>
-							<Child />
-						</span>
-					);
-				}
-			} catch (err) {
-				return <span>Error: {err.message}</span>;
-			}
-		}
-
-		renderer.render(
-			<div>
-				<Component />
-			</div>,
-			document.body,
-		);
-		expect(document.body.innerHTML).toEqual("<div><span>1</span></div>");
-		renderer.render(
-			<div>
-				<Component />
-			</div>,
-			document.body,
-		);
-		expect(document.body.innerHTML).toEqual("<div><span>2</span></div>");
-		renderer.render(
-			<div>
-				<Component />
-			</div>,
-			document.body,
-		);
-		expect(document.body.innerHTML).toEqual(
-			"<div><span>Error: errors caught and rerendered</span></div>",
-		);
-		renderer.render(
-			<div>
-				<Component />
-			</div>,
-			document.body,
-		);
-		expect(document.body.innerHTML).toEqual(
-			"<div><span>Error: errors caught and rerendered</span></div>",
-		);
-	});
-
-	test("immediate errors are caught", () => {
-		function Thrower(): never {
-			throw new Error("immediate errors are caught");
-		}
-
-		function* Child(): Generator<number> {
-			yield <Thrower />;
-		}
-
-		function* Component(): Generator<Child> {
-			try {
-				while (true) {
-					yield (
-						<span>
-							<Child />
-						</span>
-					);
-				}
-			} catch (err) {
-				return <span>Error: {err.message}</span>;
-			}
-		}
-
-		renderer.render(
-			<div>
-				<Component />
-			</div>,
-			document.body,
-		);
-		expect(document.body.innerHTML).toEqual(
-			"<div><span>Error: immediate errors are caught</span></div>",
-		);
-	});
-
-	test("async errors are caught", async () => {
-		async function Thrower(): Promise<never> {
-			throw new Error("async errors are caught");
-		}
-
-		function* Component(): Generator<Child> {
-			try {
-				yield <Thrower />;
-			} catch (err) {
-				return <span>Error: {err.message}</span>;
-			}
-		}
-
-		await renderer.render(
-			<div>
-				<Component />
-			</div>,
-			document.body,
-		);
-		expect(document.body.innerHTML).toEqual(
-			"<div><span>Error: async errors are caught</span></div>",
-		);
-		await new Promise((resolve) => setTimeout(resolve, 100));
-		expect(document.body.innerHTML).toEqual(
-			"<div><span>Error: async errors are caught</span></div>",
-		);
-	});
-
-	test("immediate async errors caught and rerendered", async () => {
-		async function Thrower(): Promise<never> {
-			throw new Error("immediate async errors caught and rerendered");
-		}
-
-		function* Component(): Generator<Child> {
-			try {
-				yield <Thrower />;
-			} catch (err) {
-				return <span>Error: {err.message}</span>;
-			}
-		}
-
-		renderer.render(
-			<div>
-				<Component />
-			</div>,
-			document.body,
-		);
-		await renderer.render(
-			<div>
-				<Component />
-			</div>,
-			document.body,
-		);
-		expect(document.body.innerHTML).toEqual(
-			"<div><span>Error: immediate async errors caught and rerendered</span></div>",
-		);
-		await new Promise((resolve) => setTimeout(resolve, 100));
-		expect(document.body.innerHTML).toEqual(
-			"<div><span>Error: immediate async errors caught and rerendered</span></div>",
-		);
-	});
-
-	test("immediate async errors are caught", async () => {
-		async function Thrower(): Promise<never> {
-			throw new Error("immediate async errors are caught");
-		}
-
-		function* Child(): Generator<number> {
-			yield <Thrower />;
-		}
-
-		function* Component(): Generator<Child> {
-			try {
-				while (true) {
-					yield (
-						<span>
-							<Child />
-						</span>
-					);
-				}
-			} catch (err) {
-				return <span>Error: {err.message}</span>;
-			}
-		}
-
-		await renderer.render(
-			<div>
-				<Component />
-			</div>,
-			document.body,
-		);
-		expect(document.body.innerHTML).toEqual(
-			"<div><span>Error: immediate async errors are caught</span></div>",
-		);
-		await new Promise((resolve) => setTimeout(resolve, 100));
-		expect(document.body.innerHTML).toEqual(
-			"<div><span>Error: immediate async errors are caught</span></div>",
-		);
-	});
-
-	test("async errors are caught in nested component", async () => {
-		async function Thrower(): Promise<never> {
-			throw new Error("async errors are caught in nested component");
-		}
-
-		function* Child(): Generator<Child> {
-			yield <span>1</span>;
-			yield <span>2</span>;
-			yield <Thrower />;
-		}
-
-		function* Component(): Generator<Child> {
-			try {
-				while (true) {
-					yield <Child />;
-				}
-			} catch (err) {
-				return <span>Error: {err.message}</span>;
-			}
-		}
-
-		await renderer.render(
-			<div>
-				<Component />
-			</div>,
-			document.body,
-		);
-		expect(document.body.innerHTML).toEqual("<div><span>1</span></div>");
-		await renderer.render(
-			<div>
-				<Component />
-			</div>,
-			document.body,
-		);
-		expect(document.body.innerHTML).toEqual("<div><span>2</span></div>");
-		await renderer.render(
-			<div>
-				<Component />
-			</div>,
-			document.body,
-		);
-		expect(document.body.innerHTML).toEqual(
-			"<div><span>Error: async errors are caught in nested component</span></div>",
-		);
-		await new Promise((resolve) => setTimeout(resolve, 100));
-		expect(document.body.innerHTML).toEqual(
-			"<div><span>Error: async errors are caught in nested component</span></div>",
-		);
-	});
-
 	test("multiple iterations without a yield throw", () => {
 		let i = 0;
 		function* Component(this: Context) {
@@ -1143,6 +849,7 @@ describe("sync generator component", () => {
 		expect(i).toBe(1);
 	});
 
+	// TODO: it would be nice to test this like other components
 	test("for await...of throws", async () => {
 		let ctx: Context;
 		function* Component(this: Context): Generator<null> {
diff --git a/src/index.ts b/src/index.ts
index 132f6e5a7..e964553ab 100644
--- a/src/index.ts
+++ b/src/index.ts
@@ -753,7 +753,10 @@ class ComponentNode<T> extends ParentNode<T> {
 					this.iterator = value;
 				} else if (isPromiseLike(value)) {
 					this.componentType = AsyncFn;
-					const pending = value.then(() => undefined); // void :(
+					const pending = value.then(
+						() => undefined,
+						() => undefined,
+					); // void :(
 					const result = value.then((child) => this.updateChildren(child));
 					return [pending, result];
 				} else {
@@ -814,7 +817,13 @@ class ComponentNode<T> extends ParentNode<T> {
 		this.enqueuedPending = undefined;
 		this.enqueuedResult = undefined;
 		if (this.componentType === AsyncGen && !this.finished && !this.unmounted) {
-			this.run();
+			// We catch and rethrow the error to trigger an unhandled promise rejection.
+			// We should probably do a larger audit of error handling and making sure error messages are helpful, but this
+			(this.run() as Promise<any>).catch((err) => {
+				if (!this.updating) {
+					throw err;
+				}
+			});
 		}
 	}
 
@@ -888,12 +897,12 @@ class ComponentNode<T> extends ParentNode<T> {
 			return;
 		}
 
+		this.updating = false;
 		this.unmounted = true;
 		this.ctx.clearEventListeners();
 		if (!this.finished) {
 			this.finished = true;
-			// TODO: maybe we should return the async iterator rather than
-			// republishing props
+			// helps avoid deadlocks
 			if (this.publish !== undefined) {
 				this.publish(this.props!);
 				this.publish = undefined;
@@ -920,6 +929,12 @@ class ComponentNode<T> extends ParentNode<T> {
 		) {
 			return super.catch(reason);
 		} else {
+			// helps avoid deadlocks
+			if (this.publish !== undefined) {
+				this.publish(this.props!);
+				this.publish = undefined;
+			}
+
 			return new Pledge(() => this.iterator!.throw!(reason))
 				.then((iteration) => {
 					if (iteration.done) {