Allows contexts to escape to parent via return #87

jnicklas · 2020-03-27T12:58:10Z

When a context returns another context, that context is kept alive. Meaning that unlike other children, it won't be halted when the context exits, it is then linked to the parent. This provides a desired escape hatch to allowing contexts escape the scope in which they were created.

Initially we used the suspend pattern for this purpose, but as documented in #86, this pattern does not compose well, and its behaviour is confusing at times.

In the future, we might extend this functionality with what we have dubbed resources, where a resource basically bundles some JavaScript object, and an associated context. This PR does not implement such a feature yet.

This PR also makes a few other changes which were necessary or convenient for implementing this feature:

execution contexts now track whether they are required. This simplifies things, since we want to preserve whether they are required when they are returned
execution contexts are not initialized with a parent, but rather they can be linked to another context through link and unlink. This allows for more flexibility
linked children always propagate errors to their parents. Instead of this being part of the functionality of spawn and fork. This does require some trickery to make generator work, but I think it is worth it.
monitor has been renamed to spawn (an alias is retained for now), because it does not add any functionality on top of context.spawn.
It is currently no longer supported to use a fork or spawn as the top level operation. Because it hurts my brain and I can't figure out what sensible behaviour actually should be.

github-actions · 2020-03-27T12:59:35Z

A preview package of this pull request has been published with the tag context-return.
You can try it out by running the following command:

$ npm install effection@context-return

or by updating your package.json to:

{
  "effection": "context-return"
}

Generated by 🚫 dangerJS against 97ef634

When a context returns another context, that context is kept alive. Meaning that unlike other children, it won't be halted when the context exits, it is then linked to the parent. This provides a desired escape hatch to allowing contexts escape the scope in which they were created. Initially we used the `suspend` pattern for this purpose, but as documented in #86, this pattern does not compose well, and its behaviour is confusing at times. In the future, we might extend this functionality with what we have dubbed resources, where a resource basically bundles some JavaScript object, and an associated context. This PR does not implement such a feature yet. This PR also makes a few other changes which were necessary or convenient for implementing this feature: - execution contexts now track whether they are required. This simplifies things, since we want to preserve whether they are required when they are returned - execution contexts are not initialized with a parent, but rather they can be linked to another context through `link` and `unlink`. This allows for more flexibility - linked children always propagate errors to their parents. Instead of this being part of the functionality of spawn and fork. This does require some trickery to make generator work, but I think it is worth it. - `monitor` has been renamed to `spawn` (an alias is retained for now), because it does not add any functionality on top of `context.spawn`. - It is currently no longer supported to use a `fork` or `spawn` as the top level operation. Because it hurts my brain and I can't figure out what sensible behaviour actually should be.

jnicklas · 2020-03-27T13:02:53Z

packages/effection/src/context.js

@@ -113,7 +112,7 @@ Thanks!`);
    if (this.isRunning) {
      this.result = value;
    }
-    if (this.requiredChildren.size > 0) {
+    if (Array.from(this.children).some((c) => (!this.parent || c !== value) && c.isRequired)) {


This conditional is quite complicated. Not sure how to reformulate this in a better way though.

jnicklas · 2020-03-27T13:03:24Z

packages/effection/src/context.js

@@ -148,14 +149,43 @@ Original error:`);
      }
    }

+    if(this.result instanceof ExecutionContext) {


Is instanceof the right choice here?

jnicklas · 2020-03-27T13:04:14Z

packages/effection/src/control.js

-        if (child.isBlocking) {
-          self.context.requiredChildren.add(child);
-        }


I don't really understand this conditional or what it is/was supposed to do.

jnicklas · 2020-03-27T13:05:22Z

packages/effection/tests/order-of-operations.test.js

@@ -24,17 +24,15 @@ describe('Order of Shutdown Operations', () => {
  beforeEach(() => {
    order = [];

-    let self = ({ resume, context: { parent }}) => resume(parent);


The reason I had to rewrite this is because returning the parent here actually transfers ownership of it now 😱

jnicklas · 2020-03-27T13:06:28Z

packages/effection/tests/execution.test.js

-
-  describe('forking from a process', () => {
-    let exec, resume, fail;
-
-    beforeEach(() => {
-      exec = main(fork(context => ({ resume, fail } = context)));
-    });
-
-    it('makes the external process waiting and blocking', () => {
-      expect(exec.isBlocking).toBe(true);
-      expect(exec.isWaiting).toBe(true);
-    });
-
-    describe('resuming the forked process', () => {
-      beforeEach(() => {
-        resume();
-      });
-      it('completes the external process', () => {
-        expect(exec.isCompleted).toBe(true);
-      });
-    });
-
-    describe('failing the forked process', () => {
-      beforeEach(() => {
-        fail(new Error('boom!'));
-      });
-
-      it('fails the process', () => {
-        expect(exec.isErrored).toBe(true);
-      });
-      it('has the error as the result', () => {
-        expect(exec.result).toBeDefined();
-        expect(exec.result.message).toEqual('boom!');
-      });
-    });
-  });
 });


For some reason figuring out how to make this work really hurt my brain. But then I realized that maybe this just shouldn't be allowed as a pattern? Ideally it would just work, but it turned out to be surprisingly tricky.

jnicklas · 2020-04-03T08:33:26Z

Closing this in favour of #89, since I did a ton of additional work which also fixed a lot of bugs in this PR.

jnicklas requested a review from cowboyd March 27, 2020 12:58

jnicklas force-pushed the context-return branch from ff838f1 to 97ef634 Compare March 27, 2020 13:00

jnicklas commented Mar 27, 2020

View reviewed changes

jnicklas added the experimental Experimental feature which should be evaluated first label Mar 27, 2020

This was referenced Mar 27, 2020

fork and spawn terminology is the wrong way around 🤔 #88

Closed

Implement support for resources #89

Merged

jnicklas closed this Apr 3, 2020

cowboyd deleted the context-return branch March 30, 2021 13:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allows contexts to escape to parent via return #87

Allows contexts to escape to parent via return #87

jnicklas commented Mar 27, 2020

github-actions bot commented Mar 27, 2020 •

edited

Loading

jnicklas Mar 27, 2020

jnicklas Mar 27, 2020

jnicklas Mar 27, 2020 •

edited

Loading

jnicklas Mar 27, 2020

jnicklas Mar 27, 2020

jnicklas commented Apr 3, 2020

Allows contexts to escape to parent via return #87

Allows contexts to escape to parent via return #87

Conversation

jnicklas commented Mar 27, 2020

github-actions bot commented Mar 27, 2020 • edited Loading

jnicklas Mar 27, 2020

Choose a reason for hiding this comment

jnicklas Mar 27, 2020

Choose a reason for hiding this comment

jnicklas Mar 27, 2020 • edited Loading

Choose a reason for hiding this comment

jnicklas Mar 27, 2020

Choose a reason for hiding this comment

jnicklas Mar 27, 2020

Choose a reason for hiding this comment

jnicklas commented Apr 3, 2020

github-actions bot commented Mar 27, 2020 •

edited

Loading

jnicklas Mar 27, 2020 •

edited

Loading