🐛 fix resource destruction order (#1091)

There was a regression introduced with
#1081 such that when a task was
halted, its resources were being shut down _before_ the task body was
finished. This resulted in violation of the structured concurrency
guarantees that required everything in scope to remain alive until
after the body of the task is fully executed. This is because the body
of the operation may use resources that are in scope throughout the
_entirety_ of the operation. For example, in the following code, the
finally block of the main operation requires the `child` task to be
fully complete.

```ts
let task = run(function* () {
  let synchronize = withResolvers<void>();
  let child = yield* spawn(() => synchronize.operation);
  try {
    yield* suspend();
  } finally {
    synchronize.resolve();
    yield* child;
  }
});

await task.halt() // deadlock!
```

Because the main task dependes on `child`, child needs to be active
and only halted _after_ the main task is complete, _not_ before. (in
this case the antecedent resource is a simple spawned `Task`, but it
could be anything; a database connection, file handle, whatever...).
Only after all references to the resource have become inactive can the
resource be destroyed.

When destroying a scope then, we need to first destroy oneself by
interrupting effect iteration, and only once that is complete, destroy
child scopes. This is accomplished by not reversing scope destructors.

It turns out this regression was actually being surfaced by the an
existing testcase which we commented out 😬. Another test has been
added to ensure the proper shutdown order is preserved when explicitly
halting.

> Note: in the future we will simplify this process even further by
removing the scope "destructor" mechanism altogether and replacing it
with middleware. This will replace fiddling with the order of
registration and execution with explicit declaration of sequence of
operations as "around" or "within" other operations.

Author: cowboyd

lib/scope-internal.ts

@@ -74,7 +74,7 @@ export function createScopeInternal(
     unbind();
     let outcome = Ok();
     try {
-      for (let destructor of [...destructors].reverse()) {
+      for (let destructor of destructors) {
         try {
           destructors.delete(destructor);
           yield* destructor();

test/resource.test.ts

@@ -107,6 +107,37 @@ describe("resource", () => {
 
     expect(sequence).toEqual([1, 2, 3, 4]);
   });
+
+  it("is released in the reverse order from which it was acquired", async () => {
+    let sequence: string[] = [];
+
+    await run(function* () {
+      yield* resource<void>(function* (provide) {
+        try {
+          yield* provide();
+        } finally {
+          sequence.push("first start");
+          yield* sleep(5);
+          sequence.push("first done");
+        }
+      });
+      yield* resource<void>(function* (provide) {
+        try {
+          yield* provide();
+        } finally {
+          sequence.push("second start");
+          yield* sleep(10);
+          sequence.push("second done");
+        }
+      });
+    });
+    expect(sequence).toEqual([
+      "second start",
+      "second done",
+      "first start",
+      "first done",
+    ]);
+  });
 });
 
 function createResource(container: State): Operation<State> {

test/run.test.ts

@@ -211,10 +211,7 @@ describe("run()", () => {
     await expect(task).rejects.toMatchObject({ message: "halted" });
   });
 
-  // TODO: this test is of dubious value. Deadlock might be the right thing here
-  // we can revisit and try to either detect this and deal with it, or maybe raise
-  // an error
-  it.skip("can halt itself between yield points", async () => {
+  it("can halt itself between yield points", async () => {
     let task: Task<void> = run(function* root() {
       yield* sleep(0);
 

test/spawn.test.ts

@@ -225,4 +225,34 @@ describe("spawn", () => {
 
     expect(order).toEqual(["zero", "one", "two"]);
   });
+
+  it("preserves all child tasks until after the parent operation passes out of scope", async () => {
+    let sequence: string[] = [];
+
+    let task = run(function* () {
+      yield* spawn(function* () {
+        try {
+          yield* suspend();
+        } finally {
+          sequence.push("first");
+        }
+      });
+      yield* spawn(function* () {
+        try {
+          yield* suspend();
+        } finally {
+          sequence.push("second");
+        }
+      });
+      try {
+        yield* suspend();
+      } finally {
+        sequence.push("parent");
+      }
+    });
+
+    await task.halt();
+
+    expect(sequence).toEqual(["parent", "second", "first"]);
+  });
 });