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

Please remove the ExecutionContext.global warning #4670

Closed
alexandru opened this issue May 7, 2022 · 4 comments
Closed

Please remove the ExecutionContext.global warning #4670

alexandru opened this issue May 7, 2022 · 4 comments
Labels
as-designed The observed behavior is as-designed, it need not be fixed.

Comments

@alexandru
Copy link

I'm seeing this warning in my project:

[error] ... The global execution context in Scala.js is based on JS Promises (microtasks).
[error] Using it may prevent macrotasks (I/O, timers, UI rendering) from running reliably.
[error]
[error] Unfortunately, there is no way with ECMAScript only to implement a performant
[error] macrotask execution context (and hence Scala.js core does not contain one).
[error]
[error] We recommend you use: https://github.com/scala-js/scala-js-macrotask-executor
[error] Please refer to the README.md of that project for more details regarding
[error] microtask vs. macrotask execution contexts.
[error]
[error] If you do not care about macrotask fairness, you can silence this warning by:
[error] - Adding @nowarn("cat=other") (Scala >= 2.13.x only)
[error] - Setting the -P:scalajs:nowarnGlobalExecutionContext compiler option (Scala < 3.x.y only)
[error] - Using scala.scalajs.concurrent.JSExecutionContext.queue
[error]   (the implementation of ExecutionContext.global in Scala.js) directly.
[error]
[error] If you do not care about performance, you can use
[error] scala.scalajs.concurrent.QueueExecutionContext.timeouts().
[error] It is based on setTimeout which makes it fair but slow (due to clamping).
[error]
[error]             Future(1).map { x =>
[error]                   ^

I understand the intent, or why usage of scala.concurrent.ExecutionContext.global may be problematic, however it's a standard import that often gets used for code that cross-compiles to both the JVM and JS, which is one of the primary strengths of Scala.js. Having the official compiler perpetually warn on standard functionality, and suggest a third-party library, isn't good IMO. And why isn't that warning and option available on Scala 3.x?

Personally, I see only 3 possibilities:

  1. Fix global in Scala.js proper;
  2. Deprecate global and remove it completely in a future version;
  3. Leave it as is, and remove the warning;

As it is, removing that warning is a lot of work, especially in a project that compiles for multiple Scala versions. Updating minor Scala.js versions shouldn't be this hard.

Just a suggestion, thanks a lot for your work 🤗

@sjrd
Copy link
Member

sjrd commented May 7, 2022

Have you read #4129, which led to this warning? There is a lot of context in there that explains how we got to make this decision. Do you have any new information that would invalidate the reasoning made there?

@alexandru
Copy link
Author

Hi @sjrd,

I remember that issue, I even added some input at that point — which was that, out of all solutions, continuing with Promise.then is probably the least desirable solution, being non-standard and leaky.

My problem with it is that it's violating the principle of the least surprise, because people that want to use global (or Future), expect fairness guarantees, not performance. Seeing it used in Scala.js was a surprise to me, because in my JavaScript days I've never thought of using it like that.

When importing global, I would expect it to use setTimeout. It's the most obvious implementation for when setImmediate is not available, as that's what people used and still use in the browser. Also, I did some measurements on Node.js, and the clamping on successive is around 1ms (instead of the usual 4ms, which is what happens in browsers). Not great, but not terrible.

The issue I'm seeing is with the behavior of Future. After the BatchedExecutor optimizations from Scala 2.13.2, the behavior of global would be less relevant. However, AFAIK, after this issue was reported, the global optimizations were reverted, people expected to import ExecutionContext.batched. Which for Future should be another way to solve performance issues, if global would actually use setTimeout.

Speaking of, what sense does ExecutionContext.batched make in Scala.js, given that global is implemented with Promise.then? The two may be equivalent in Scala.js, but they shouldn't be, as trampolining Runnable tasks still makes sense.

In the browser, at least, setTimeout(0) is throttled for UI responsiveness. And I remember that the reason for why setImmediate never happened as a standard was due to setTimeout(0) being enough, and it did not make sense to throttle setTimeout while introducing a setImmediate workaround, which would have ended up throttled as well. It's what people used for making their callbacks stack-safe, prior to the introduction of async/await and Promise.

In my opinion, setTimeout is perfectly acceptable.

But if it isn't, due to performance reasons, then own the current implementation, instead of triggering a warning.

If Future really is the equivalent of Promise, then it needs to be usable out of the box, with no warnings.

Triggering a warning on usage of ExecutionContext.global is like providing the user with a button, and then complaining when the button gets pressed. Like, don't provide the button, if the implementation is that terrible. Otherwise own it.

I'm basically complaining about usability here.

@gzm0
Copy link
Contributor

gzm0 commented May 8, 2022

I have taken a stab at grouping the discussion here abit and giving my POV.

Usability

And why isn't that warning and option available on Scala 3.x?

Fair point, but IIUC feature-parity (and compiler option syntax) between Scala 2.x / 3.x are a more general issue.

however it's a standard import that often gets used for code that cross-compiles to both the JVM and JS, which is one of the primary strengths of Scala.js. Having the official compiler perpetually warn on standard functionality, and suggest a third-party library, isn't good IMO.

Absolutely. This isn't good. But IMHO it's the least bad we could come up with. So unless we have a better option (see second section below), I do not know what you want us to do.

Deprecate global and remove it completely in a future version;

That's essentially what the warning is (also see response below). Actually removing it is a bit tricky because it's in the Scalalib, which the Scala.js project doesn't directly control. In any case, a new major Scala.js version is not on the horizon any time soon, so IMHO, no point in figuring out how to remove it right now.

Like, don't provide the button, if the implementation is that terrible.

Fair. But you need to think about this more like a deprecation warning (we cannot remove it due to backwards compatibility guarantees). The reason it isn't directly implemented as a deprecation warning is due to how Scala.js itself cross compiles the scala library.

Alternatives

being non-standard

Could you clarify what you mean by non-standard? IIUC, Promise.then is part of the ECMAScript standard.

and leaky

nodejs/node#6673 (comment)

suggests that the issues you point out depend on the exact usage of the API and are not inherent to using Promise.then. Whether or not the Scala.js implementation exposes this leak, I do not know. But if it does, that is a bug and we should fix it.

When importing global, I would expect it to use setTimeout
In my opinion, setTimeout is perfectly acceptable.

See: #4129 (comment)

Please address this point when you're arguing for using setTimeout.

If Future really is the equivalent of Promise, then it needs to be usable out of the box, with no warnings.

If Future were the (full) equivalent of js.Promise it wouldn't even try to offer fairness guarantees, just like js.Promise.

Ownership

But if it isn't, due to performance reasons, then own the current implementation, instead of triggering a warning.

I'm not 100% sure what you mean by "owning' here. We maintain (to the best of our abilities) both implementations.

Batched Execution

Speaking of, what sense does ExecutionContext.batched make in Scala.js, given that global is implemented with Promise.then?

I do not know.

@alexandru
Copy link
Author

alexandru commented May 20, 2022

IIUC, Promise.then is part of the ECMAScript standard.

The signature, yes, the implementation, no — there are 3 major browser engines with 3 different implementations of Promise.then, with slightly different contracts implemented (last time I checked, maybe that changed, but I seriously doubt it).

If Future were the (full) equivalent of js.Promise it wouldn't even try to offer fairness guarantees, just like js.Promise.

Right. Well, it would also leak in flatMap "tail-recursive" loops, but only on Chrome and Firefox, not Safari.

I'm not 100% sure what you mean by "owning' here. We maintain (to the best of our abilities) both implementations.

"Owning it" as in living with your choice, with no regrets 🙂

image


I think this is a contentious issue for a usability concern, and recommending that project to people is useful enough, so I'm going to backtrack on my suggestion.

Cheers,

@sjrd sjrd added the as-designed The observed behavior is as-designed, it need not be fixed. label Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
as-designed The observed behavior is as-designed, it need not be fixed.
Projects
None yet
Development

No branches or pull requests

3 participants