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

Experiment: Only allow Async.Spawn to spawn runnable futures #46

Merged
merged 6 commits into from
Mar 11, 2024

Conversation

natsukagami
Copy link
Contributor

@natsukagami natsukagami commented Feb 27, 2024

What's this?

The goal is to disallow spawning dangling Futures from using Async functions.
Async.Spawn is an opaque alias of Async, defined as a subtype of Async,
obtained by explicitly "upgrading" it through Async.group -- or automatically given
through Async.blocking or Future.apply.

The Async.Spawn-taking functions (signalling usage of dangling futures)
should follow the hacky signature of Future.apply:

  def apply[T](body: Async.Spawn ?=> T)
    (using async: Async, spawn: Async.Spawn & async.type): T

to ensure that the given Async instance (which is usually synthesized
to be the innermost context) is the same instance as the Async.Spawn
instance. It happens quite often (especially when nesting Async contexts)
that these don't match:

  extension[T] (s: Seq[T])
    def parallelMap[U](f: T => Async ?=> U)(using Async): Seq[U]
 
  Async.blocking: // Async.Spawn here...
    val seq = Seq(1, 2, 3, 4, 5)
      .parallelMap: n => // Async here...
         Async.select(
            Future(doSomethingAsync(n)) handle Some(_),     // oops, spawned by Async.blocking
            Future(Async.sleep(1.minute)) handle _ => None, // oops, spawned by Async.blocking
         )
    // oops, leaking all the futures...

with the Future.apply signature as above, this does not happen and will
give a compile time error.

The goal is to disallow spawning dangling Futures from `using Async` functions.
`Async.Spawnable` is an opaque alias of `Async`, defined as a subtype of `Async`,
obtained by explicitly "upgrading" it through `Async.spawning` (which works similarly
to `Async.group`, so futures are all cancelled after) -- or automatically given
through `Async.blocking` or `Future.apply`.

The `Async.Spawnable`-taking functions (signalling usage of dangling futures)
should follow the hacky signature of `Future.apply`:

> def apply[T](body: Async.Spawnable ?=> T)
>   (using async: Async, spawn: Async.Spawnable & async.type): T

to ensure that the given `Async` instance (which is usually synthesized
to be the innermost context) is the same instance as the `Async.Spawnable`
instance. It happens quite often (especially when nesting `Async` contexts)
that these don't match:

> extension[T] (s: Seq[T])
>   def parallelMap[U](f: T => Async ?=> U)(using Async): Seq[U]
>
> Async.blocking: // Async.Spawnable here...
>   val seq = Seq(1, 2, 3, 4, 5)
>     .parallelMap: n => // Async here...
>        Async.select(
>           Future(doSomethingAsync(n)) handle Some(_),     // oops, spawned by Async.blocking
>           Future(Async.sleep(1.minute)) handle _ => None, // oops, spawned by Async.blocking
>        )
>   // oops, leaking all the futures...

with the `Future.apply` signature as above, this does not happen and will
give a compile time error.
@adamw
Copy link

adamw commented Feb 28, 2024

Not really related to the PR, rather to the general design - out of curiosity, why do you need the context parameter in def parallelMap[U](f: T => Async ?=> U)(using Async): Seq[U]? If f would spawn any async computations, it could capture the capability from the enclosing environment, at usage-site, no?

In ox we have a similar method, but the signature is simpler (it's a top-level function, not an extension, but that's irrelevant I guess): def mapPar[I, O, C[E] <: Iterable[E]](parallelism: Int)(iterable: => C[I])(transform: I => O): C[O]. So I think we avoid this problem (if I understand the problem correctly), but maybe we have some other problems that I don't know about ;)

@natsukagami
Copy link
Contributor Author

natsukagami commented Feb 28, 2024 via email

@adamw
Copy link

adamw commented Feb 28, 2024

Ah yes I see the use-case and problem :)

I guess I would typically expect f to create its own scope, e.g. (using ox's syntax - I think supervised is more or less Async.blocking):

myList.parallelMap { n =>
  supervised { // custom scope
    val f1 = fork(...)
    val f2 = fork(...)
    f1.join() + f2.join()
  }
}

That way if a particular mapping invocation is interrupted (e.g. one of the f invocations throws an exception), this will propagate to interrupt whatever is happening in the supervised.

But still it's possible that when parallelMap is itself inside a scope, it will capture the wrong one:

supervised {
  myList.parallelMap { n =>
    val f1 = fork(...)
    val f2 = fork(...)
    f1.join() + f2.join()
  }
}

Now an exception thrown by any of the f invocations would interrupt the forks, thus ending the outer scope, which is probably not what you'd want. I wonder if capture checking would be able to solve this - we'd have to require that f does not capture Ox/Async.

But I guess that's what you are trying to solve here, in another way? One thing I don't understand - isn't Async and Async.Spawnable really two different capabilities?

@natsukagami
Copy link
Contributor Author

isn't Async and Async.Spawnable really two different capabilities?

They are quite different from gears's POV I think, due to how gears think of concurrency as suspendable computations rather than just scoped threads.

  • Async encapsulates the ability of a computation to suspend itself to wait for some concurrently arriving value (the .await in Async). To do so it needs to have both the capability to be paused and to be put in queue to resume later.
  • However, the "common" case of waiting for some value usually comes in parallel with the ability to create more concurrent computations (i.e. spawning runnable Futures) to begin with. This is what Async.Spawnable gives you on top of Async.

Initially I think it is totally fine to keep both of the capabilities within Async, but it seems to undermine principles of structured concurrency, especially when calling an using Async function might leave you with futures still running after it returns.

I don't think there is a concept of .await in Ox (we just rely on blocking ops in Loom JVM being able to handle suspension), and so the two looks the same.

I think supervised is more or less Async.blocking

With the above in mind I think it is more clear that Async.spawnable is closer to supervised ;)

There is not that much value in *not* giving `Spawnable` within `Async.group`,
so we should just merge the two together.
@adamw
Copy link

adamw commented Feb 29, 2024

@natsukagami Thanks, a great explanation! Indeed, that's where ox/gears differ: in ox, there's no capability needed to block (suspend) - you can always do that. So we only have the other one (scoping threads).

shared/src/main/scala/async/Async.scala Outdated Show resolved Hide resolved
shared/src/main/scala/async/futures.scala Outdated Show resolved Hide resolved
shared/src/main/scala/async/futures.scala Outdated Show resolved Hide resolved
shared/src/main/scala/async/futures.scala Show resolved Hide resolved
Should make it more compatible with capability-speak.
... to be contrasted with Threads and be more in line with gear's Timer and Multiplexer APIs.
@natsukagami natsukagami changed the title [WIP] Experiment: Only allow Async.Spawnable to spawn runnable futures Experiment: Only allow Async.Spawnable to spawn runnable futures Mar 5, 2024
@natsukagami natsukagami marked this pull request as ready for review March 5, 2024 14:11
@natsukagami natsukagami changed the title Experiment: Only allow Async.Spawnable to spawn runnable futures Experiment: Only allow Async.Spawn to spawn runnable futures Mar 5, 2024
@natsukagami natsukagami requested a review from m8nmueller March 6, 2024 14:11
natsukagami added a commit that referenced this pull request Mar 7, 2024
With #46 and #48 we will have some breaking changes merged in.
Co-authored-by: Maximilian Müller <m8n.mueller@gmail.com>
@natsukagami
Copy link
Contributor Author

Ok, let's get this in ;)

@natsukagami natsukagami merged commit f66a696 into lampepfl:main Mar 11, 2024
3 checks passed
@natsukagami natsukagami deleted the spawnable-async branch March 11, 2024 14:45
natsukagami added a commit to amsen20/web-crawlers-bench that referenced this pull request May 14, 2024
`using Async.Spawn` by itself does *not* obey the normal Scoping rules for
Async contexts. See lampepfl/gears#46.
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.

3 participants