Add exit for use in SuspendApp

[alexandru]

Related to: #3846

This may be premature, and please feel free to ignore my PR, especially as it proposes the introduction of a new public function.

I want a way to exit a SuspendApp with a certain exit code (non-zero exit code), and without logging an exception (stderr output should be in my control); so without halt, such that graceful shutdown should still work.

[nomisRev]

Hey @alexandru,
Thank you for the PR, and adding your findings in the issue. I agree it's very meaningful to be able to exit with a custom exit code but I originally wanted to avoid CoroutineScope.() -> ExitCode.

The implementation looks goot but I'm a bit unsure about the implicit API by using CoroutineContext, I'd love it if we could do it in a typesafe way by changing the current signature which can be done in a source compatible and non-binary breaking way.

public interface SuspendAppScope : CoroutineScope {
  public fun exit(code: Int)
}

public fun SuspendApp(
  context: CoroutineContext = Dispatchers.Default,
  uncaught: (Throwable) -> Unit = Throwable::printStackTrace,
  timeout: Duration = Duration.INFINITE,
  block: suspend SuspendAppScope.() -> Unit,
): Unit = TODO("Existing implementation with updated receiver")

@Deprecated(
  message = "Use SuspendApp with SuspendAppScope instead",
  level = DeprecationLevel.HIDDEN
)
public fun SuspendApp(
  context: CoroutineContext = Dispatchers.Default,
  uncaught: (Throwable) -> Unit = Throwable::printStackTrace,
  timeout: Duration = Duration.INFINITE,
  block: suspend CoroutineScope.() -> Unit,
): Unit = SuspendApp(context, uncaught, timeout) {
  block()
}

WDYT?

[kyay10]

That doesn't actually work because function parameter types are erased on the JVM. You could add a dummy parameter to the new overload, but actually, none of that is necessary. Changing the receiver type to a more specific type like that should be perfectly binary compatible, I believe

[alexandru]

I'll try it out, see what works.

[nomisRev]

You're right @kyay10 it creates a JVM clash so we would have to introduce a @JvmName for the new SuspendApp function which I guess is not very problematic since I highly doubt this will be used from Java.

Alternatively and maybe better like @kyay10 suggested we just replace the current receiver from CoroutineScope -> SuspendAppScope which is binary compatible on the JVM but not binary compatible for klib. Since this function is used pretty much only be end-users and not libraries I think this is acceptable.

Does a receiver DSL solve your problem as well @alexandru? Or did you purposefully design it through a CoroutineContext?

[kyay10]

Another option is to use a context parameter alongside the CoroutineScope receiver, now that they're stable. That'd be a different function arity, so it wouldn't cause a signature clash, and it's a nice way to compose things instead of having an uber-receiver

[alexandru]

I would very much like a DSL, but I worry about binary backwards compatibility.

The problems happen not for first party code, but rather when updated as a tranzitive dependency. I'm not convinced that going from CoroutineScope to SuspendAppScope is binary compatible, because that's a function parameter, so for binary compatibility we need to respect contravariance.

Maybe there's something I don't understand here, but given that the current type for that input is this:

block: suspend CoroutineScope.() -> Unit

What happens when Arrow gets updated as a transitive dependency, it now expects a suspend SuspendedAppScope.() -> Unit on that function invocation, but current code passes in a suspend CoroutineScope.() -> Unit? The JVM bytecode is probably going to be fine, since its type erased anyway, but at runtime this may turn out to be a class cast exception (dynamic linking strikes again).

[kyay10]

I won't belabor the JVM type erasure point, since we agree on that.
The idea here is to have SuspendAppScope: CoroutineScope, so a function like suspend CoroutineScope.() -> Unit can obviously be called with a SuspendAppScope, hence there cannot be any class cast exceptions at runtime.

Because of contravariance, suspend SuspendAppScope.() -> Unit is actually a less-specific type than suspend CoroutineScope.() -> Unit

[nomisRev]

@alexandru this is the change in JVM bytecode

public final class arrow/continuations/SuspendAppKt {
	public static final fun SuspendApp-exY8QGI (Lkotlin/coroutines/CoroutineContext;Lkotlin/jvm/functions/Function1;JLkotlin/jvm/functions/Function2;)V
	public static synthetic fun SuspendApp-exY8QGI$default (Lkotlin/coroutines/CoroutineContext;Lkotlin/jvm/functions/Function1;JLkotlin/jvm/functions/Function2;ILjava/lang/Object;)V
}

+public abstract interface class arrow/continuations/SuspendAppScope : kotlinx/coroutines/CoroutineScope {
+	public abstract fun exit (I)V
+}

This is the change in klib:

// Klib ABI Dump
// Targets: [iosArm64, iosSimulatorArm64, iosX64, js, linuxArm64, linuxX64, macosArm64, macosX64, mingwX64, tvosArm64, tvosSimulatorArm64, tvosX64, wasmJs, watchosArm32, watchosArm64, watchosSimulatorArm64, watchosX64]
// Rendering settings:
// - Signature version: 2
// - Show manifest properties: true
// - Show declarations: true

// Library unique name: <io.arrow-kt:suspendapp>
+abstract interface arrow.continuations/SuspendAppScope : kotlinx.coroutines/CoroutineScope { // arrow.continuations/SuspendAppScope|null[0]
+    abstract fun exit(kotlin/Int) // arrow.continuations/SuspendAppScope.exit|exit(kotlin.Int){}[0]
+}

-final fun arrow.continuations/SuspendApp(kotlin.coroutines/CoroutineContext = ..., kotlin/Function1<kotlin/Throwable, kotlin/Unit> = ..., kotlin.time/Duration = ..., kotlin.coroutines/SuspendFunction1<kotlinx.coroutines/CoroutineScope, kotlin/Unit>) // arrow.continuations/SuspendApp|SuspendApp(kotlin.coroutines.CoroutineContext;kotlin.Function1<kotlin.Throwable,kotlin.Unit>;kotlin.time.Duration;kotlin.coroutines.SuspendFunction1<kotlinx.coroutines.CoroutineScope,kotlin.Unit>){}[0]
+final fun arrow.continuations/SuspendApp(kotlin.coroutines/CoroutineContext = ..., kotlin/Function1<kotlin/Throwable, kotlin/Unit> = ..., kotlin.time/Duration = ..., kotlin.coroutines/SuspendFunction1<arrow.continuations/SuspendAppScope, kotlin/Unit>) // arrow.continuations/SuspendApp|SuspendApp(kotlin.coroutines.CoroutineContext;kotlin.Function1<kotlin.Throwable,kotlin.Unit>;kotlin.time.Duration;kotlin.coroutines.SuspendFunction1<arrow.continuations.SuspendAppScope,kotlin.Unit>){}[0]

You can test this yourself by running ./gradlew :suspendapp:updateLegacyAbi. I totally agree that we should not break binary compatibility, and it's always my biggest concern. Maybe I rely to much on the binary-compatibility-validator, and we need to manually double check.

[kyay10]

I've been thinking about this for a while. I wonder if we can automate some experimental binary compatibility tests. We can keep around the test jars from previous releases and make sure it all runs successfully (we might also need to change around our tests to have some flag for experimental APIs so their tests don't run for the binary compatibility checks). Effectively, we would have "proof" that, if an API has tests going back to version X, we're still binary compatible with it

Of course, this should be an addition to the BCV, not instead of it.

This might be way too ambitious for this change, though.

I'm fairly confident that this change will be binary compatible (on JVM, to be clear). An easy way to see it is if we break it down into 2:

• We add SuspendAppScope, but we don't change any signatures. We simply pass SuspendAppScope where we previously passed CoroutineScope
• We change the signature from CoroutineScope.() -> Unit to SuspendAppScope.() -> Unit. There are no behaviour changes here, just a signature change.

Step 1 doesn't change any signatures, and it doesn't change the behaviour of the CoroutineScope passed in, so it's not a "behaviour change" in any way we care about (like sure, technically, if someone was doing this::class.simpleName, they'd get different behaviour). Step 2 changes no behaviour, and only updates the signature, but not in a way that changes the erased signature at all. Again, because the behaviour didn't change, we couldn't, all of a sudden, magically get CCEs (in fact, I'm pretty sure the JVM bytecode is unchanged by step 2)

Contrast this with an absolutely breaking change: changing the receiver to String or something. You can do it in one shot, and nothing will show up on the BCV, but it's absolutely a breaking change of behaviour and will result in CCEs. Importantly, you can't do it in 2 steps (one for behaviour, and one for signature) like I've done above.

[alexandru]

@kyay10 Having tests for binary compatibility would be great. For Scala, we have the Mima plugin, which takes a different approach than Kotlin's ABI validator, as it downloads a previous version and looks at bytecode signatures, although TBH I like Kotlin's ABI validator more.

On the topic of the PR, I think my intuition was wrong, will test it when I get to my laptop; as I'm all for the DSL.

[alexandru]

I made the changes, let me know what you think

[nomisRev]

Looks good to me @alexandru. Good KDoc as well ❤️
Thank you so much for your first contribution and support to Arrow-kt 🙌

[kyay10]

We might wanna consider adding a SuspendAppExit interface so that context users can use context(_: CoroutineScope, _: SuspendAppExit) if they so wish, but that can always come later

[serras]

I think it would be possible to maintain binary compatibility here:

• Give a different JvmName to the version using SuspendAppScope,
• Create a new overload using CoroutineScope without JvmName (so the name is maintained).

[kyay10]

This maintains JVM compatibility. I do agree that it breaks Klib compatibility, but we deemed that to be okay (see main conversation).

Simply, SuspendAppScope implements CoroutineScope, so no CCE will happen. Function types are erased on JVM, so no ABI breaks happen

[serras]

Please take my suggestion as a simple change that would bring 100% fidelity in JVM. But I don't want it to devolve into discussion about what exactly counts as binary compatible (e.g., should we take reflection into account?), so I'm also fine leaving it as is.

[alexandru]

Please don't merge it yet, sorry, there's one last thing to settle here...

---

Function input parameters are contravariant, meaning that, because SuspendAppScope extends CoroutineScope, then suspend CoroutineScope.() -> Unit extends (or is a subtype of) suspend SuspendAppScope.() -> Unit, so whenever a suspend SuspendAppScope.() -> Unit is required, we can supply a value of type suspend CoroutineScope.() -> Unit.

This is why this code should stay compatible, even if Arrow's SuspendApp builder gets upgraded:

val myFun: suspend CoroutineScope.() -> Unit = TODO()
//...
SuspendApp(myFun)

Contravariance is somewhat counterintuitive, but we should be good. The JVM would consider this backwards compatible even if type erasure wouldn't be a thing.

---

OTOH, I'm having doubts about SuspendAppContext being an interface with abstract methods. I think it should be a final class and exit an abstract method.

The reason is that adding abstract methods to an interface does break binary compatibility in a big way.

[nomisRev]

As @serras already mentioned maybe we should aim to keep 100% fidelity and JVM & KLIB compatibility by keeping the old function with @Deprecated("...", level = DeprecationLevel.HIDDEN) and add @JvmName("...") to the new function. That will without any doubt preserve all edge-cases. Sorry for leading in a different direction I originally wanted to prevent keeping both APIs but I am now failing to come up with a good argument now.

---

Good point. In Ktor for example we keep all interfaces small or SAM, and add the functionality as extensions.

I think it should be a final class and exit an abstract method.

Not entirely sure what you mean by this? 🤔 I am open to having SuspendAppScope as a class this is uncommon for now in Arrow-kt, and has some other trade-offs, but would allow us to extend the DSL in the future.