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

.all() creates a new Scope #897

Open
charles-toller opened this issue Feb 17, 2024 · 2 comments
Open

.all() creates a new Scope #897

charles-toller opened this issue Feb 17, 2024 · 2 comments
Milestone

Comments

@charles-toller
Copy link

I'm not sure what the reasoning behind this is, but it appears that .all() creates a new Scope when it's called, but isn't documented as such. This can cause surprising behavior: an example from some code I was writing tonight looks something like this:

const ops: Operation<int>[] = [/* some array of operations that should run simultaneously */];
const tasks = yield* all(ops.map((op) => spawn(() => op))); /* throws error: Halted, surprising */
for (const task of tasks) {
    const result = yield* task;
    /* do something async with result, say:*/
    yield* sleep(result);
}

I believe this is because the operations all start, returning a task, but when the Scope from all is destroyed as a result of it returning to the parent, the scope for all the spawned operations is also dead.

If I just wanted to run them simultaneously and deal with the results, I could just use .all directly and skip this, but in this case I want to run all the operations simultaneously and then do something else asynchronous with them.

In any case, this is workaround-able by simply emulating what .all does without creating a new scope, but it seems strange to me that the new scope is needed in the first place, as it seems to be just an extra call wrapping the whole function.

Is this intentional behavior?

@cowboyd
Copy link
Member

cowboyd commented Feb 19, 2024

@charles-toller Have you considered using Scope.run()? You could use it to create your concurrent tasks in the current scope, and then do more async stuff with them.

const scope = yield* useScope();
const ops: Operation<int>[] = [/* some array of operations that should run simultaneously */];
const tasks = yield* ops.map((op) => scope.run(() => op));
for (const task of tasks) {
    const result = yield* task;
    /* do something async with result, say:*/
    yield* sleep(result);
}

While that's the workaround I would employ, that didn't really answer your question. The answer is a bit complex.

Yes, it is indeed the the way it was written, but that is not to say that it couldn't or shouldn't change.

There were several reasons to give all() its own scope.

  1. symmetry with race(). Race gets its own scope because you want to discard all the losers once a winner has been determined. The mechanism used to do this was scope. This is not necessary with all() because there are no losers.
  2. logically grouping all tasks run via all() into a single node in the tree, that way when we write our inspector, you can see where they came from.

But there is a deeper problem. Even if all() did not have a call() wrapped around it, there is still the fact that it uses spawn() internally which means that each operation will run in a spawned scope. if you spawn within that spawned scope, then your task will still exit immediately because there is no computation to perform.

One option would be that if an operation returns a Task, then that task is "adopted" by the caller. This idea would need to be vetted, but it does make some sort of sense, because what would ever be the use-case of returning a "dead" task?

currently:

let task = yield* spawn(function*() {
  return yield* spawn(function*() {
    yield* sleep(10);
    yield* return "hello from the inside";
  });
});
console.log(yield* task); //=> Error('halted');

We could change that to work..

However, there is a challenge that might make this infeasible, and that is how would you handle tasks that are embedded in other values? I.e. what do you do when you have tasks wrapped in an array?

let task = yield* spawn(function*() {
  let one = spawn(function*() {
    yield* sleep(10);
    yield* return "hello from the inside ONE";
  });
  let two = spawn(function*() {
    yield* sleep(10);
    yield* return "hello from the inside TWO";
  });
  return [one, two];
});
console.log(yield* task);

Alternatively, we could add an "adopt" operation to Scope that would allow you to write your example like this:

const scope = yield* useScope();
const ops: Operation<int>[] = [/* some array of operations that should run simultaneously */];
const tasks = yield* all(ops.map((op) => scope.adopt(() => op)));
for (const task of tasks) {
    const result = yield* task;
    /* do something async with result, say:*/
    yield* sleep(result);
}

As you can see, there are actually some very tricky concerns here, but I do agree with you that the most intuitive feeling thing when you see:

all(ops.map((op) => spawn(() => op)));

Is that they are spawned in the current scope.

@cowboyd
Copy link
Member

cowboyd commented Feb 19, 2024

@sivakumar-kailasam this is a great example of how having native delimited continuations would solve this problem very cleanly. If we had it, then we could write all() directly to have the desired semantics without having to add another clever mechanism like "adopt" or "keepalive" via return value.

export function* all(ops) {
  return yield* shift<All<T>>(function*(k) {
    let resolved: All<T> = [];
    for (let op of ops) {
      yield* reset(function*() {
	resolved.push(yield* op());
	if (resolved.length === ops.length) {
	  yield* k(resolved);
	}
      });
    }
  });
}

@cowboyd cowboyd added this to the v3.1 milestone Feb 28, 2024
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

No branches or pull requests

2 participants