nadako / haxe-coroutines

41 stars 5 forks source link

Is implicit suspension really such a good idea? #10

Open back2dos opened 6 years ago

back2dos commented 6 years ago

The reason I'm kind of worried about this is that I can see myself using it on a nodejs server to write request handlers that are run with high concurrency.

The thing I like about explicit use about a callback, promise or await is that looking at the code I can see that execution is suspended. I think it's pretty important to see these places, because it's exactly the ones where race conditions may occur in an execution model such as node's (or that of a browser). On a large enough project the implicitness may create more problems than it solves. If for some reason (e.g. a large enough team) any of the functions called from a suspend function becomes suspendable itself, the change passes silently, even though it potentially introduced a bug that only surfaces under high concurrency and therefore in all likelihood won't be caught by tests either and instead occasionally occurs in production - the kind of bug that nobody wants to have to deal with.

I can see the appeal of the implicitness and in many scenarios I think I would use it myself (e.g. in functional code or in small scripts). So I wonder if we could somehow make this an opt-in/opt-out. Perhaps something like using ImplicitSuspend;/using ExplicitSuspend; would be good, because then you can make it per module or even per folder with import.hx.

delahee commented 6 years ago

I like the opt in/out idea. It is also a great debugging tool and allow people to experiment easily.

nadako commented 6 years ago

The idea with the current proposal is already opt-in explicitness implemented through a normal suspending function called await.

E.g.

import PromiseAwait.await;

function makeRequest():Promise; 

suspend function someFunction() {
    var data = await(makeRequest()); // or makeRequest().await() with `using`
}

Isn't that explicit enough?

back2dos commented 6 years ago

Ok, let me rephrase that: what options does a 20+ person team have to make sure everything is explicit within a specific critical part of the code?

Imagine this:

suspend function someFunction() {
  var foo = getFoo();
  var bar = await(getBar());
  ...
}

Everybody reading this will think that getFoo is sync. But you have to look up getFoo to be sure. And you have to potentially do it every time you check out somebody else's new code.

nadako commented 5 years ago

So this basically comes down to a whitelist of suspending functions that are allowed to be directly called. I'm pretty sure this can be implemented on top of this proposal, even with a macro (but of course it would be faster if implemented in compiler).

Let's leave this open so we don't forget to experiment with it.

back2dos commented 5 years ago

Hmm, perhaps I'm misunderstanding. Will suspension only be implicit within functions marked with suspend (or whatever the keyword)? The README doesn't say that, but OTOH I don't see how it would be possible otherwise. If so, then I withdraw my objection.

Simn commented 4 years ago

That sounds good to me. Any suspend function can implicitly suspend further calls, but if you do it from a "normal" function you have to be explicit.

back2dos commented 3 years ago

After giving this more thought, I would like to ask for an option for disabling implicit suspension after all.

In theory, this feature seems like a good fit for asys (or just nodejs). So much so, that it's conceivable that one would want to build an entire server with it (and indeed it's not uncommon to see nodejs servers built almost entirely on async function).

In such a scenario, I think it's important to be able to enforce a coding style where suspension must be explicit, because while each suspension-free section is atomic, every suspension yields control, thereby opening the door for race conditions. This isn't much of a problem for dialogs/transitions, but on a server handling thousands of concurrent users, one careless suspension can lead to bugs. The type of bugs that only occur in production under load.

In a code review, it would therefore be nice to have them stand out. And the possibility of silently introducing a new implicit suspension into the codebase merely by updating a dependency only to find out in production is also one less thing I'd like to worry about.

Simn commented 3 years ago

Note that the "stand out" part doesn't necessarily mean that it has to be written in the code. As long as the compiler, and thus the IDE, know what suspends and what doesn't, this information can be shown to the user in some way.

I guess we could provide the option to force explicit suspension, but in that case it should be package-based (like null-safety) so one can still use dependencies that don't adhere to this limitation.

nadako commented 3 years ago

In Kotlin there's a feature called restricted suspension that kind of provides something like that - it forbids calling any suspend functions that are not methods of the given "receiver" (another kotlin feature, which is basically configurable this). I say "kind of", because it's not really meant as a code style enforcement tool, but rather a way to forbid calling suspend methods that can break the semantics of some specific coroutine use-case like generators with yield.

We can think of some metadata that could provide such restriction in different scopes (package-level, class-level, method-level, expression-level), then it would be possible to use this for both code style enforcement and scenarios like generator+yield.

(this might be actually already be possible with a macro)

Gama11 commented 3 years ago

Speaking of Kotlin, pretty much every single Kotlin developer uses IntelliJ, and it shows these icons in the gutter:

image