SendChannel.offer should never throw

[LouisCAD]

As of version 1.1.1, if you call offer from the non blocking world (which doesn't handle CancellationException by default, on a closed Channel, an exception is thrown, which by default will crash the program.

This is a problem in several ways:

1. offer which returns a Boolean per its signature actually is kind of a "tri-lean" as if the channel is closed, it will not return false but throw.
2. offer is possibly called from a thread that is not the one where the callback that offers the values is unregistered, so race conditions may imply offer being called once or a few times after the channel is closed, and usually, the intention is to ignore those. Simply returning false instead of throwing would likely be a better behavior.
3. There's no way to offer without having this function possibly throw, because by the time isClosedForSend returns false, it may have been closed due to concurrent execution, so a subsequent call to offer would throw.

My current workaround is the following:

fun <E> SendChannel<E>.offerCatching(element: E): Boolean {
    return runCatching { offer(element) }.getOrDefault(false)
}

[elizarov]

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

[LouisCAD]

I think the "OrClosed" suffixed functions would greatly fit the use cases where you need to know the channel was closed.
However, there are some cases where handling the closed case is not desirable and ignoring it is preferable (think interop with callback hell based APIs that can call back on an unspecified thread).

If the "OrClosed" suffixed functions make it to a release, would the offer function (and maybe poll although less useful IMHO) change its behavior to stop throwing when the channel is closed and return false instead?

[elizarov]

I don't have an answer on the last question yet. Indeed, we are running into this and some other issues that might require us tweaking semantics of channels with respect to the close operations, however it is not clear how we could introduce those changing without breaking a lot of code.

[LouisCAD]

I wonder if someone ever caught that closed exception satisfying a use case with this behavior.

[elizarov]

Somebody might have been offering elements to a channel until it is closed relying on an exception to abort this offering loop on channel close.

[LouisCAD]

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).

[ansman]

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.

[LouisCAD]

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

[elizarov]

OrClosed, unfortunately, is not stable yet. It's ABI will change.

[LouisCAD]

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.

[psteiger]

+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.

[RBusarow]

You can just use this:

fun <E> SendChannel<E>.sendBlockingOrNull(element: E) = try {
  sendBlocking(element)
} catch (e: ClosedSendChannelException) {
  null
}

But I agree, I wish offer would just fail safely.

[LouisCAD]

I use runCatching around offer (or a try catch), because I don't need to launch a coroutine in callbacks.

[eygraber]

Got to this issue via @LouisCAD from a post on the Kotlin slack channel. I reported getting crashes on my Android app for a CancellationException with no stacktrace;

 kotlinx.coroutines.JobCancellationException: Job was cancelled; job=JobImpl{Cancelling}@57b9ffd

This is a huge project, that was built with kotlin and coroutines from the ground up. There's heavy usage of offer (for better or for worse) and to handle each case would be a giant undertaking, let alone first figuring out where this crash is coming from.

Granted, I never read the docs closely enough to notice the caveat about throwing if it's closed, but I assumed it was a safe call because it returns Boolean.

[LouisCAD]

As said on Slack:

You can use "Replace in Path" to replace with something like the extension I show in the initial post of the issue. Then, all you'll need is to fix the imports. If you don't use star imports, that just means using "Replace in Path" for the import as well, and then, ensure all the modules of your project build.

[pacher]

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

[martinbonnin]

I've just been bitten by that too (See apollographql/apollo-kotlin#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.

[YuriDenison]

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?

[qwwdfsad]

We still willing to fix it in 1.4.x

[nachtien]

@qwwdfsad any progress on this? My team won't let me use callbackFlow because it is marked as experimental as a result of this issue. 🥺

[LouisCAD]

@nachtien this issue has a workaround. Unless you need binary compatibility reaching very very far into the future, I see no reason to not use callbackFlow and workaround that issue.

Experimental is for the API, it doesn't mean the thing is specially risky to use or that it doesn't work.

[nachtien]

Right, but my team says "no experimental ever", no matter what I say, thus i can't use this.

[zach-klippenstein]

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.

[LouisCAD]

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 😄

[LouisCAD]

Is the plan of resolution for this issue to evolve the API so it uses Kotlin 1.5 value classes with the @JvmInline optimization?

[qwwdfsad]

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.

[fvasco]

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.

[tarifchakder]

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