Convert bindAll to an async function

[wch]

This closes #3899, but is currently just a draft PR because there's an issue that needs to be addressed with it.

The problem was the following: onValueChange is an async function, but there was one place where it was getting called without an await. This allowed multiple calls to renderValue to happen concurrently, which caused the bad behavior.

This PR addresses the issue by converting that call to onValueChange() to await onValueChange(), and making all the functions on the call stack also async, and using await to call all of them.

The result of all this is that the externally-facing function Shiny.bindAll has changed from sync to async. Overall, this is an improvement from Shiny 1.7.5, because the code that's called from that function has been fixed so that all async functions are awaited. However, compared to Shiny 1.7.4, it could still be a step back because in that version, the caller of bindAll could expect that function to work synchronously -- all of its effects would be completed when the function returns.

One possible way of dealing with this is to do the same pattern that we used in render.ts, where we kept old functions like renderHtml and provided new functions with the suffix Async, like renderHtmlAsync. If we do this, then we would end up with a new function called bindAllAsync (along with some others that would be on the call stack). We could also deprecate bindAll and encourage extension authors to use bindAllAsync.

Another possibility is to leave the code as it is in this PR, and tell extension authors that bindAll() is now an async function.

One other note: after enabling the no-floating-promises eslint rule, I found that there were a number of other places where we create promises without awaiting them, and those should be addressed in another PR.

[cpsievert]

If we do this, then we would end up with a new function called bindAllAsync (along with some others that would be on the call stack).

Would bindAllAsync() be the only addition to the public API? If so, it seems I'd be in favor of having both bindAllAsync() & bindAll() (not only to mitigate regression risk, but also for consistency with the rest of the API)

[wch]

In addition to bindAllAsync(), I think we would also need to:

• Rename OutputBinding.onValueChange (which is async) to OutputBinding.onValueChangeAsync, and add a synchronous version of OutputBinding.onValueChange
• Rename HtmlOutputBinding.renderValue (which is async) to HtmlOutputBinding.renderValueAsync, and add a synchronous version of HtmlOutputBinding.renderValue

And we'd also need sync/async versions of the functions that have been touched in this PR.

It might be simpler to find packages that use Shiny.bindAll() and make PRs converting them to call await Shiny.bindAll(). That change would also work when used with older versions of Shiny, because in JS, you can use await on a synchronous function.

[cpsievert]

I don't think the addition of HtmlOutputBinding.renderValueAsync/OutputBinding.onValueChangeAsync would be that bad since they're both essentially private (i.e., >95% of bindings shouldn't need to call or implement these methods)?

That said, I'm not totally against changing bindAll() to be async, but if we do go in that direction, seems like unbindAll() should also be async?

[gadenbuie]

Could bindAll() leverage the taskQueue() for scheduling/managing the async onValueChange() calls in a way that could avoid needing async bindAll()?

Another thought that's less likely to pan out

The other vague intuition I had while debugging this issue is that there's both asynchronous programming and recursion at play. It might have been a detail particular to the example we were looking at, but I found myself wondering if the .onValueChange() call is completely necessary. IIUC, we can enter a loop where onValueChange() calls renderHtmlAsync() which calls bindAll() which bindOutputs() which calls onValueChange()... maybe there's some level of uneccessary recursion or a recursion edge case we could exclude?

Something else we might want to look at: we protect ourselves from the double bind in bindOutputs() by checking for the shiny-bound-output class, but bindOutput() throws if the output id is already known to the binding. But maybe there's room to adjust these checks? The second check just looks at this.$bindings[id], but we could follow that up with a check to see if the element with id was already bound to the binding of bindOutput(), in which case maybe we could no-op.

[wch]

I don't think the addition of HtmlOutputBinding.renderValueAsync/OutputBinding.onValueChangeAsync would be that bad since they're both essentially private (i.e., >95% of bindings shouldn't need to call or implement these methods)?

That's true, it won't add too much complexity. However, something that we should think about is how to deal with bindings that can only be implemented with async versions of these functions.

In terms of sync/async renderValue/onValueChange methods, the three possibilities are:

1. A binding which only has a synchronous implementation (this encompasses most bindings today). Sync functions can be called from async functions, so these bindings would work in both cases.
2. A binding which has separate sync and async implementations.
3. A binding which only has an async implementation (this includes the current implementation of HtmlOutputBinding; see below).

For example, in Shinylive (for both R and Python), HtmlOutputBinding will only work properly with async, because in Shinylive it can only load JS resources asynchronously on Chromium-based browsers. (This is because Chrome doesn't allow service workers to intercept synchronous XMLHttpRequests and it looks like they're never going to fix that. https://bugs.chromium.org/p/chromium/issues/detail?id=602051)

The category that would have problems is (3). Also note that if there are any bindings that currently are in category (1) but do async loading of resources, they won't work in Shinylive.

After writing this all out, part of me thinks that we should have the separate sync/async methods, but part of me also just wants to rip off the band-aid and make everything async so that we don't have to maintain parallel code paths indefinitely into the future.

That said, I'm not totally against changing bindAll() to be async, but if we do go in that direction, seems like unbindAll() should also be async?

Yes, for symmetry, I think it would make sense to mark unbindAll() as async but I don't think we'd need to change any code inside of it.

[wch]

Could bindAll() leverage the taskQueue() for scheduling/managing the async onValueChange() calls in a way that could avoid needing async bindAll()?

Unfortunately, no... The main issue is that when bindAll() is called, the caller might expect that when it returns, it has completed all of its work, including loading JS assets. For example, their code might call bindAll() an then immediately start doing something with the output, like fetch its value, but that would fail if the JS hasn't been loaded yet.

With all the async/promise stuff going on under the hood, the only (hypothetical) way to guarantee that everything has completed when bindAll() returns is if the (synchronous) call to bindAll() were to block until the internal async function calls are finished. But in JS, it's not possible for a sync function to block until promises/async function calls are completed.

Another thought that's less likely to pan out
The other vague intuition I had while debugging this issue is that there's both asynchronous programming and recursion at play. It might have been a detail particular to the example we were looking at, but I found myself wondering if the .onValueChange() call is completely necessary. IIUC, we can enter a loop where onValueChange() calls renderHtmlAsync() which calls bindAll() which bindOutputs() which calls onValueChange()... maybe there's some level of uneccessary recursion or a recursion edge case we could exclude?

Something else we might want to look at: we protect ourselves from the double bind in bindOutputs() by checking for the shiny-bound-output class, but bindOutput() throws if the output id is already known to the binding. But maybe there's room to adjust these checks? The second check just looks at this.$bindings[id], but we could follow that up with a check to see if the element with id was already bound to the binding of bindOutput(), in which case maybe we could no-op.

If I'm understanding correctly, I think that these two possibilities don't quite get to the root of the problem -- the double binding of an output is really more of a symptom of a deeper problem, which is that, currently, the code that handles the bindings is not guaranteed to execute in any particular order because we launched a Promise and didn't wait for it to finish. Because the code is not guaranteed to execute in a particular order, it becomes very hard to reason about, and multiple concurrent "tasks" could be doing overlapping operations on the same data structures, which could violate assumptions about how the code is supposed to work. Making it fully async-await will guarantee that execution order is linear.

[gadenbuie]

Thanks for considering my comments and for your replies, @wch. I think the fact that bindAll() is semi-public forces our hand here; if it were internal only I think we might be able to use the taskQueue to enforce execution order, but that wouldn't resolve the issue of public usage.

To clarify my above points: if bindAll() pushed its work into the taskQueue(), we would avoid the problems in #3899, i.e. those where we call bindAll() as part of an update cycle. But that would still make bindAll() an async function, causing issues for anyone calling bindAll() in a script and expecting it to be synchronous.

Personally, I'm in favor of making bindAll() an async function directly. I think parallel methods add considerable complexity with little gain, especially given that the parallel approach breaks down quickly since some onValueChange() methods can only be implemented as async functions.

From a quick scan of GitHub search results, I think most people are already calling bindAll() in a way that's compatible with a fire-and-forget async call. On the other hand, many people call unbindAll() before bindAll(). My sense is that making unbindAll() async would have a bigger impact than bindAll().