Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement support for resources #89

Merged
merged 9 commits into from
Apr 15, 2020
Merged

Implement support for resources #89

merged 9 commits into from
Apr 15, 2020

Conversation

jnicklas
Copy link
Collaborator

@jnicklas jnicklas commented Mar 30, 2020

A resource ties an execution context to an object. This allows an object to be passed around between contexts while retaining an associated context. This context is "kept alive" as long as the object's scope is alive. If the object is returned out of the current scope, the context is hoisted to the parent scope along with the object. This allows an escape hatch from contexts being local to where they were created and makes it possible to implement the RAII pattern.

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.
A resource ties an execution context to an object. This allows an object to be passed around between contexts while retaining an associated context. This context is "kept alive" as long as the object's scope is alive. If the object is returned out of the current scope, the context is hoisted to the parent scope along with the object. This allows an escape hatch from contexts being local to where they were created and makes it possible to implement the RAII pattern.
@jnicklas jnicklas added the experimental Experimental feature which should be evaluated first label Mar 30, 2020
@jnicklas jnicklas requested a review from cowboyd March 30, 2020 12:08
@dep dep bot added the dependent label Mar 30, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Mar 30, 2020

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

$ npm install effection@resources

or by updating your package.json to:

{
  "effection": "resources"
}

Generated by 🚫 dangerJS against 3f5581c

The first implementation using an unassociated ExecutionContext turned out to be problematic, because forks created within such a child ended up without a parent. This implementation instead creates a hidden parent context for all generator operations, which are then children of this context. We are overriding `trapExit` to customize what happens when a child is finished. This also means we can get rid of the odd continuation bit in ExecutionContext.

This makes top level forks work as intended.

There is a bit of a weird workaround to make `fork` implementable as a regular operation:

If a required context is returned, it will NOT block until completed, instead it will be passed to the parent in its current state. I think this is reasonably intuitive. The weirdness comes in that if a required context is returned from the root context, then it WILL block. I think in practice this is probably fine, but it is a bit odd.
@jnicklas
Copy link
Collaborator Author

jnicklas commented Apr 7, 2020

Just a note that this is definitely a breaking change, did find a few places in bigtest where this was breaking things in subtle ways. Mostly where we were returning a context for no real reason, which is now doing things when previously it was just useless.

jnicklas added a commit to thefrontside/bigtest that referenced this pull request Apr 7, 2020
Effection now supports resource objects on an experimental branch, this makes it possible to tie the lifetime of a context to an object, and return it out of its context to pass ownership of the context to the parent. This provides an alternate escape hatch to using suspend, which is more disciplined and composes better.

See: thefrontside/effection#89
jnicklas added a commit to thefrontside/bigtest that referenced this pull request Apr 7, 2020
Effection now supports resource objects on an experimental branch, this makes it possible to tie the lifetime of a context to an object, and return it out of its context to pass ownership of the context to the parent. This provides an alternate escape hatch to using suspend, which is more disciplined and composes better.

See: thefrontside/effection#89
jnicklas added a commit to thefrontside/bigtest that referenced this pull request Apr 8, 2020
Effection now supports resource objects on an experimental branch, this makes it possible to tie the lifetime of a context to an object, and return it out of its context to pass ownership of the context to the parent. This provides an alternate escape hatch to using suspend, which is more disciplined and composes better.

See: thefrontside/effection#89
jnicklas added a commit to thefrontside/bigtest that referenced this pull request Apr 8, 2020
Effection now supports resource objects on an experimental branch, this makes it possible to tie the lifetime of a context to an object, and return it out of its context to pass ownership of the context to the parent. This provides an alternate escape hatch to using suspend, which is more disciplined and composes better.

See: thefrontside/effection#89
jnicklas added a commit to thefrontside/bigtest that referenced this pull request Apr 8, 2020
Effection now supports resource objects on an experimental branch, this makes it possible to tie the lifetime of a context to an object, and return it out of its context to pass ownership of the context to the parent. This provides an alternate escape hatch to using suspend, which is more disciplined and composes better.

See: thefrontside/effection#89
jnicklas added a commit to thefrontside/bigtest that referenced this pull request Apr 9, 2020
Effection now supports resource objects on an experimental branch, this makes it possible to tie the lifetime of a context to an object, and return it out of its context to pass ownership of the context to the parent. This provides an alternate escape hatch to using suspend, which is more disciplined and composes better.

See: thefrontside/effection#89
jnicklas added a commit to thefrontside/bigtest that referenced this pull request Apr 9, 2020
Effection now supports resource objects on an experimental branch, this makes it possible to tie the lifetime of a context to an object, and return it out of its context to pass ownership of the context to the parent. This provides an alternate escape hatch to using suspend, which is more disciplined and composes better.

See: thefrontside/effection#89
jnicklas added a commit to thefrontside/bigtest that referenced this pull request Apr 9, 2020
Effection now supports resource objects on an experimental branch, this makes it possible to tie the lifetime of a context to an object, and return it out of its context to pass ownership of the context to the parent. This provides an alternate escape hatch to using suspend, which is more disciplined and composes better.

See: thefrontside/effection#89
@jnicklas jnicklas marked this pull request as ready for review April 14, 2020 13:47
@dep dep bot removed the dependent label Apr 14, 2020
@jnicklas jnicklas removed the experimental Experimental feature which should be evaluated first label Apr 14, 2020
@jnicklas
Copy link
Collaborator Author

@cowboyd marking this as ready for review :)

Copy link
Member

@cowboyd cowboyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still trying to understand the difference between creating a resource and returning a resource. Is my understanding correct here?

function* operation() {
  let resource = yield function* createResource() {
    let create = resource({}, constructoOperation); //<- an operation to create a thing
    let res = yield create; //<- actually creates a thing....
    return res; //<- links to parent context.
  }
  yield somethingElse();
  return resource; //-> links to parent context
}

My biggest questions are:

  1. when should the create be considered done?
  2. is binding the resource to the current context necessary when the only purpose is to return it to the parent context?

link(child) {
if(this.id === child.id) {
throw new Error('cannot link context to itself');
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth adding in a check here to make sure that the new parent is higher in the tree than the old parent? It feels like it is not ok, to transfer ownership "down-tree", although we may want a way to do that explicitly in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was sort of what I was thinking, I think it makes sense to expose link as an operation, to essentially "move" a child somewhere, though there might be some weird implications to that which I haven't thought through, so I didn't add it in this PR. But we could definitively add such a check for now, until we're sure that it is actually safe to do this.

@@ -1,10 +1,13 @@
export { timeout } from './timeout';
export { fork, join, monitor } from './control';
export { fork, join, spawn, spawn as monitor } from './control';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should mark monitor as deprecated, maybe emit a console warning once.

Copy link
Member

@cowboyd cowboyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See questions on bigtest repo, but looks good to me overall. Nice work!

@jnicklas
Copy link
Collaborator Author

1. when should the `create` be considered done?

A resource starts off in the context where it was created, and it effectively functions like a child of this context, so when the context is finalized the resource is halted.

The one exception to this is when the child resource is returned out of the context. Any child resource or child context which is returned from a context is not halted on finalization.*

Additionally, a resource can exit early, that is if the resource's context becomes completed. This is a trivial example:

yield resource({}, function*() { return });

The associated context here does nothing, so there is nothing keeping it alive. Compare to this:

yield resource({}, function*() { yield });

I've actually thought about whether a resource which finishes early should be considered an error, as this is probably never what you want, and it's actually already bitten me once or twice, but I felt that that's maybe a tad bit too opinionated 🤷‍♂️

2. is binding the resource to the current context necessary when the only purpose is to return it to the parent context?

Unfortunately not, since everything in Effection is synchronous, whatever happens up until the first yield point will run synchronously on creation of the resource. An error which occurs here will need to be handled by ... someone. So by creating the resource in the current context, and then moving it upwards, we ensure that there is someone there who can handle this error.

* (except, and here is where things get a bit weird: if the context that returns the resource is itself a resource context or it is the root context, some other weird rules apply, which feel like they make sense, but actually kinda don't 😂. But even though the implementation differs slightly here, functionally it feels like it behaves about the same)

@jnicklas
Copy link
Collaborator Author

One thing that I just realized is that we should consider a guard against returning a context or resource which is NOT a child of the current context, while this probably works just fine, it feels like it violates the "spirit" of this API, and shouldn't be allowed.

@jnicklas jnicklas merged commit a7fa84e into master Apr 15, 2020
@jnicklas jnicklas deleted the resources branch April 15, 2020 15:15
jnicklas added a commit to thefrontside/bigtest that referenced this pull request Apr 15, 2020
Effection now supports resource objects on an experimental branch, this makes it possible to tie the lifetime of a context to an object, and return it out of its context to pass ownership of the context to the parent. This provides an alternate escape hatch to using suspend, which is more disciplined and composes better.

See: thefrontside/effection#89
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants