kotlinx.coroutines: SendChannel.offer should never throw

We’ve studied our channel API surface and tried to work towards not only with the new method name instead of offer, but rather with a consistent naming scheme that can be further used in other libraries or coroutines API. Here’s a compressed reasoning path for the new naming.

When looking at the new API with TBD names, whether it’s “catching offer” or “receiveOrClosed”, the very first thing that comes to mind is that these methods try to do the same thing – error encapsulation. Other languages have built-in constructs for that, e.g. try function in Go and try! macro in Rust (NB: replaced with trailing ? in the latest releases). A similar convention is adopted in Java as well: Spliterator.tryAdvance or Iterables.tryFind are good examples of that.

So, maybe just adding try as a method prefix would suffuce: tryOffer, trySend, tryReceive and tryPoll. It could’ve worked, but it introduces new problems and ambiguities:

• We want to deprecate offer, so eventually channels will have tryOffer, but not offer. That’s understandable for people who use coroutines for a while, but not that clear for newcomers, and the general idea will be much harder to grasp for them
• In the “concurrent” world, “try*” typically means “attempt to do the operation without blocking/suspensions”. Lock.tryLock is the biggest example here. Now having API that is both suspendable and not, it’s easy to get thinking that “trySend” is just a “send” without suspension. (or even worse, does Deferred.tryAwait suspend?)

Orthogonally, Kotlin has Result class for encapsulating either successful or failed result, and *Catching functions to encapsulate computation and its result. Moreover, we lift the restrictions regarding Result usages in Kotlin 1.5 and expect people to use it even more. The only downside is a bit too long name that may be not so convenient.

Taking it into account and the fact, that offer and poll are legacy Java-isms (attempt to mimic queue interface), we decided to stick with the following:

• Regular suspending methods are left as-is: send, receive.
• Their non-suspending counterparts with error encapsulation are consistently prefixed with try: trySend and tryReceive instead of offer and poll.
• Error-encapsulating suspending methods will have the suffix catching. For now we only provide receiveCatching and onReceiveCatching (instead of previously internal receiveOrClosed)
• All error-encapsulating methods return either kotlin.Result (e.g. Deferred.awaitCatching) or domain-specific result class. For channels it is ChannelResult.

We still willing to fix it in 1.4.x

I’ve been bitten by that again just now (plus earlier)!

I know a bunch of code using callbackFlow/channelFlow in the wild is unsafe as well because offer throws as soon as the channel is closed, which happens before the block for awaitClose is executed.

That causes problems as the unregister can happen after the channel is closed if it’s not taking place on the same thread and awaitClose is used (which is often the case for both).

Please, can you integrate a safe alternative to that before Kotlin 1.4?

It’ll be almost one year I reported this issue in less than a month.

my team says “no experimental ever”, no matter what I say, thus i can’t use this.

There’s 4 other possibilities if they don’t try to understand what experimental stands for in this particular case:

1. Fire the team
2. Leave the team
3. Put as many team members on your side as possible, and convince others nicely, or disregard their objection
4. Introduce a plugin that opts-in sneakily via freeCompilerArgs so they don’t see the warning.

Disclaimer: These might all be bad ideas 😄

I think I prefer offer over trySend naming because it doesn’t bring confusion with send, which has a very different behavior.

So, my proposal is to add an overload with the following signature: offer(e: Element, throwIfClosed: Boolean), and deprecate offer(e: Element), with a ReplaceWith clause set to offer(e, throwIfClosed = true).

That avoids instantiating a sealed class, and that still makes developers upgrading kotlinx.coroutines notice the previous offer calls could have been throwing.

What do you think? 👍 or 👎

I have had several bugs in production because of this, it’s especially bad in Android where it will likely crash the app and since Kotlin doesn’t have checked exceptions it’s very easy to forget even if you’ve been bitten before. I agree that it doesn’t seem very likely that anyone is relying on this behavior.

You could always fork this library and remove the experimental annotations in your fork, then use that. Then nothing will change by surprise, ever! You just then have to keep pulling changes from this repo into your own, but you’d have full control over what changes to bring in.

I would not recommend this approach generally (because, as Louis said, there’s no real reason to not use experimental APIs in something that’s not a library), but if you have very strong restrictions around API stability, it’s one potential solution.

Yes, we are aiming to solve it in 1.4.

Please postpone the PR: we neither have resources right now to properly review it in a reasonable timeframe nor ready to do a thoughtful design meeting (to align potential trySend with potential receiveOrClosed, tryReceive and similar pattern in kotlinx-datetime).

I’ll keep you updated when we start discussing it.

@eygraber Non-trowing offer seems to be what many people expect by analogy from BlockingQueue, so the only way out I see here is to deprecate it.

Here’s a better replacement plan. Let’s introduce a single trySend function that returns TrySendResult sealed class with 3 cases: Success, Failure, and Closed. We’ll add ifClosed inline extension to this result class and will suggest trySend().ifClosed { throw it } as an offer replacement. That makes the throwing nature of the original offer totally explicit. You’ll be able to fix the throwing by simply removing .ifClosed { throw it } part.

This is only my two cents, but I think it’s more likely that people have been bitten by offer unexpectedly throwing like I have than people having found the current behavior useful and having relied on it. Also, I think people would have relied on offer throwing would read the release notes before migrating to a newer version of kotlinx.coroutines (although it’s arguable it could happen from under with a transitive dependency if they’re lagging behind), more so since using Channels currently often implies using @ObsoleteCoroutinesApi annotated symbols, which add warnings unless you opt-in.

That’s why I think adding offerOrClosed and making offer no longer throw when the channel is closed, but instead return false would be an acceptable resolution of that issue, which possible negative impact should be limited, if any.

The benefit to addressing this issue that way IMHO are that Channels API would be simpler and have a more predictable behavior, giving you the choice on how you deal with closed channel (and avoiding possible crashes when calling offer out of suspending functions, that are hard to debug in my experience).

We can actually have a slightly different proposal in mind:

suspend fun <E> ReceiveChannel<E>.receiveOrClosed(): ValueOrClosed<E>
suspend fun <E> SendChannel<E>.sendOrClosed(element: E): ValueOrClosed<Unit>
fun <E> SendChannel<E>.offerOrClosed(element: E): ValueOrClosed<Boolean>

where a ValueOrClosed<T> is a special class we are going to introduce that contains either a value of type T or a closed token.

See also #330

UPDATED: It will have to be a sealed class, but we can use inline function here:

sealed class TrySendResult {
    object Accepted : TrySendResult()
    object Rejected : TrySendResult()
    data class Closed(val cause: Throwable) : TrySendResult()
}

inline fun TrySendResult.isAcceptedOrIfClosed(onClosed: (Throwable) -> Boolean): Boolean = when(this) {
    is TrySendResult.Accepted -> true
    is TrySendResult.Rejected -> false
    is TrySendResult.Closed -> onClosed(cause)
}

Then current offer(x) call gets cleanly replaced with:

trySend(x).isAcceptedOrIfClosed { throw it }

It is Ok that it is long. The best part is that you immediately see what it does without looking at the docs.

Note, that closing causing of the Channel can be any Throwable, not necessarily a CancellationException, and a usual attempt to send or offer over a “failed” channel would throw that exception.

`fun <E> SendChannel<E>.safeOffer(value: E) = !isClosedForSend && try {
    offer(value)
} catch (e: CancellationException) {
    false
}`

All error-encapsulating methods return either kotlin.Result (e.g. Deferred.awaitCatching) or domain-specific result class.

I strongly vote for a domain specific class, kotlin.Result is too generic to be used in every specific method.

I am not really happy of *Cathing convention, I don’t have a better proposal but I have to admit that receiveOrClosed sounds good for me.

Yes, we are aiming to solve it in 1.4.

@qwwdfsad Could you please clarify if this issue is fixed in 1.4.0, which was released today?

Status update: this issue is not abandoned and will be fixed along with Kotlin 1.4 companion release (either it will be kotlinx.coroutines 1.3.8 or kotlinx.coroutines 1.4-M1).

Unfortunately, it is not possible to release it earlier due to KT-27524, that is fixed in 1.4

@elizarov Your last snippet doesn’t compile, and I don’t really understand it either.

It just shows a proposed API (no impl)

Regarding your snippet before that you updated: If you expose a closedCause property on SendChannel, you can use it in isAcceptedOrIfClosed implementation and keep using an enum (be it in screaming case or not).

Hm. That’s an interesting idea!

I agree with @elizarov (here) and @zach-klippenstein (here).

In a -unlikely- future, we should expect also tryReceive(): ValueOrClosed

I think having an offer alternative that always allocates a sealed class on each call is not great regarding efficiency, that’s why I’m offering a way that doesn’t require it.

@LouisCAD, did you consider this implementation?

sealed class TrySendResult {
    object Accepted : TrySendResult()
    object Rejected : TrySendResult()
    class Closed(val cause: CancellationException) : TrySendResult()
}

inline fun TrySendResult.ifAccepted(block: () -> Unit): TrySendResult {
    if (this is TrySendResult.Accepted) block()
    return this
}

inline fun TrySendResult.ifRejected(block: () -> Unit): TrySendResult {
    if (this is TrySendResult.Rejected) block()
    return this
}

inline fun TrySendResult.ifClosed(block: (CancellationException) -> Unit): TrySendResult {
    if (this is TrySendResult.Closed) block(cause)
    return this
}

Success/Failure usually imply lack of or presence of some error, which could be confusing with Closed which would be returned if the channel actually had an error. How about calling them Accepted/Rejected instead?

There’s a lot going on here and offer throwing an exception really seems to be a problem people are getting bitten by. How’s offer different from send and how we came to introduce it?

• send is suspending, so it can only be used in a suspending context where any CancellationException is always considered “normal” and ignored.
• offer was designed to be a non-suspending sibling to send, so it was designed to behave in the same way. However, we’ve failed to foresee that it is going to be used a lot in non-suspending callbacks where a CancellationException causes some real issues.

We can do a “quick-fix” by providing a replacement with essentially the same boolean-returning signature (for cases where you don’t care) that never throws. But how we should name it? The only good name I can come up with is trySend.

But what do we do with offer? As pointed out by @pacher it is not correct to simply deprecate the offer, since it is useful in actors are suspending, will properly handle CancellationException and need to be terminated promptly (without cancellation exception an offer-ing loop will just work forever). I suggest to deprecate an offer (since it’s very short name is non-explicit about its throwing behavior) and replace it with more explicitly named trySendOrThrow.

What do you think?

Please don’t change the semantics of existing API without major version change and/or deprecation cycle!

I totally agree that offer throwing on close is easy to overlook and seems annoying to handle at the first glance. But once you learn it, you start to use it and appreciate it. I am for one relying on this behavior.

For example think of actors or any other process on the other side of the channel:

1. offer returns false means it’s all good and well, but the actor is too busy at the moment (mailbox is full) and sending to the channel will block/suspend. I want to know that, so that I can choose what to do in this case, that is why I use offer instead of send in the first place
2. offer throws meaning that actor has died for some reason and I need to handle it!

These are two totally different scenarios and execution paths. If the semantics would silently change overnight combining the two, it will be much harder for me to detect and fix malfunctioning system pretending to be fine than to deal with loud and clear explicit exception. Failing fast is so much better than incorrect behavior, always.

Currently actor builder returns SendChannel, so this exception is the only way to detect problems on the other side as a client. If you think about it, present semantics of offer totally make sense, concise and good.

+1 to that. I’ve chosen to never offer anymore — I always send, and that means I (needlessly) launch a coroutine if not inside one, just to circumvent this.

But send will throw exactly the same exception if the channel is closed, how is it different? That’s the thing, we currently have offer, send and sendBlocking with consistent behavior. In my opinion either all of them should throw an exception or none of them.

I am all for introducing safer functions and very much like the offerOrClosed idea. But please add, don’t break

Returning inline classes is still experimental and will remain experimental in 1.4, so we will not be able to introduce a stable function returning Result. The same problem with xxxOrClose variants we’ve originally conceived (they are all designed to return ValueOrClosed inline class).

@eygraber I’m not quite getting how you’ve counted 3 functions. What would be the difference between “one that is meant for use in a suspending context” and “one that is meant for non-suspending context that can throw a CancellationException”. They are the same in my mind. That’s what I’ve offered (pun intended) to call trySendOrThrow.

I’ve just been bitten by that too (See https://github.com/apollographql/apollo-android/issues/2005 for some context). The offer kdoc mentions the exception but I missed it since I based my code on the callbackFlow snippet.

I’ve opened a PR there to improve the aforementioned snippet.

Now that the OrClosed functions made it to a release, can this be addressed?