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

Thoughts on async support and API implications #831

Open
mitchmindtree opened this issue Dec 30, 2021 · 1 comment
Open

Thoughts on async support and API implications #831

mitchmindtree opened this issue Dec 30, 2021 · 1 comment

Comments

@mitchmindtree
Copy link
Member

#815 adds async support in order to provide a better experience when targeting wasm+wgpu for the web, however the nested executor invocation issues (#826) show that we're only part way there to properly supporting async with nannou.

In order to allow for the creation of windows to run on the main executor, the functions in which Window::build is called should be marked as async and run in the executor's context.

While it's common to create all necessary windows during model, we also want to support the case where windows can be created at runtime, e.g. maybe pressing a button opens another control panel window.

Just marking model as async would only enable the creation of windows during model. If we want to allow users to create windows during update too, then we'd also need update to be async, which seems quite a bit trickier to achieve. update is called within the blocking call to winit::EventLoop::run callback, which is not async and already runs within the futures::executor::block_on context created on App::run.

One option might be to not run the entirety of App::run within an executor, but instead only block on the default window creation future, then return and run the main event loop afterwards outside of any executor context. This is basically the behaviour that we had pre #815 though, and I'm unsure exactly what the trade-offs are when targeting the web. @Woyten would you mind elaborating a little on how #815 enables support for the web? It might make it a little easier to hone in on a potential solution.

Alternatively, we can investigate whether or not there is an executor implementation out there (i.e. an alternative to futures::executor::block_on) that does allow nested execution?

@Woyten
Copy link
Contributor

Woyten commented Dec 31, 2021

Hi everyone!

#815 is a very incomplete first experiment which will require some massaging or an alternative solution.

As a brief summary, async functions are used in Nannou's get_or_request_async and get_or_request_device_async function which are required when creating a window. Nannou hides asynchronicity from the user by calling futures::executor::block_on s.t. windows can be created in a synchronous (but blocking) fashion. The problem arises when futures::executor::block_on is called inside a browser environment and a runtime error is raised. This is an immediate result of the Browser's single-threaded task execution model which allows for currency through promises (async functions with escape/yield points) but not through multi-threading.

Hence, to make Nannou run on the web we need to prevent calling futures::executor::block_on when a web environment is detected and call into a function that is non-blocking instead. I tried out wasm_bindgen_futures::spawn_local but async_std::task::spawn might work as well. The two main strategies to tackle the problem that come into my mind are:

Option A: Public async API

  • This is the pattern proposed in Add async twin functions for WASM support #815. Nannou never calls into futures::executor::block_on but the consumer of the final Nannou app has to either call futures::executor::block_on or wasm_bindgen_futures::spawn_local depending on their environment.
  • PRO: Very clean and idiomatic/explicit API
  • PRO: Can be implemented incrementally without breaking changes via twin functions (foo and foo_async)
  • CON: Every event handling function has to be made async in order to allow the creation of windows for any type of event
  • TODO: Resolve "Cannot execute LocalPool executor from within another executor" panic! on master #826. I would propose trying to get rid of the futures dependency and use async_std::task::block_on as well as async_std::task::spawn instead. @mitchmindtree Would it be worth trying to replace futures with async_std?

Option B: Hidden async API

  • Nannou calls into async_std::task::spawn instead of futures::executor::block_on
  • PRO: The Nannou API is unchanged, no migration required
  • PRO: Event handling functions do not have to be made async
  • CON: This will introduce a breaking change since async_std::tasks::spawn behaves differently than futures::executor::block_on. async_std::tasks::spawn will execute a little later and you cannot get synchronous feedback about whether creating a window succeeded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants