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:
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.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.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)
}
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
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?
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.
I wonder if someone ever caught that closed exception satisfying a use case with this behavior.
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.
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).
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.
Now that the OrClosed functions made it to a release, can this be addressed?
OrClosed, unfortunately, is not stable yet. It's ABI will change.
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.
+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.
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.
I use runCatching around offer (or a try catch), because I don't need to launch a coroutine in callbacks.
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.
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.
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:
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 placeoffer 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_
offeranymore — I alwayssend, 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
I think I too may have just been bitten by this when switching over from https://github.com/f2prateek/rx-preferences to https://github.com/tfcporciuncula/flow-preferences. Please see https://github.com/tfcporciuncula/flow-preferences/issues/1. When the crash happens, there really isn't any more details in the stack trace other than:
Process: <process_name>, PID: 14789
kotlinx.coroutines.JobCancellationException: FlowSubscription was cancelled; job=FlowSubscription{Cancelled}@bed4c69
It's not even about a specific library, it's about the behavior and the doc of callbackFlow that encourages to write the bug.
I regret I didn't think about submitting a change to the doc earlier, but it's now there in #1824 by Martin.
I hope it'll get merged to stop the horror stories brought by library developers that just read the misleading example in the doc.
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?
I think there should be 3 distinct functions provided to replace offer; one that is meant for use in a suspending context, one that is meant for non-suspending context that can throw a CancellationException, and one that is meant for non-suspending context that swallows the CancellationException.
For the first, I think trySend would be better, because in a suspending context we shouldn't need to explicitly know about CancellationException.
For the second, I think trySendOrThrow would be appropriate given it's behavior.
For the third, I think offer would be best, because that's how I (and it appears many others as well) initially interpreted it. In my case, I assumed the behavior because of the similarity in concept to Queue.offer. I don't know what a good name for this would be if offer could not be used off the top of my head. I changed all offer calls to offerCatching (which swallows the CancellationException) in my codebase, but I don't particularly like that name.
@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.
Yep, that's what happens when you're in a rush and type out whatever pops into your head 😁
My first 2 suggestions could be collapsed into one, and I think trySendOrThrow works well for that.
How about changing the return type instead? For example to Result type like in runCatching. This way folks who "offer and forget" can keep doing so, but will never get "_unexpected_" exception. On the other hand if somebody like me is interested in the result, (s)he will have to deal with possible exception, compiler will make sure of it.
Result already offers plenty of methods like getOrNull and getOrThrow. We can have all of them just a dot away without polluting coroutines API
Upd.: I actually like @eygraber's idea of offerCatching returning Result. It's presence alone in API should be a good indication that normal offer is throwing. But you can also deprecate offer and point to offerCatching if you please.
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).
I thought about it a little more, and I think the issue with trySendOrThrow is that it suggests that an exception would get thrown if send doesn't happen.
Ideally, offer could be overloaded to accept a duration like BlockingQueue.offer(E, Long, TimeUnit) which throws an InterruptedException, and offer wouldn't throw anything, but that would break backwards compatibility.
In any case, my main point is that because of the naming, there is an expectation that offer wouldn't throw.
@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.
That works for me!
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?
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 would prefer to not have a parameter dictate the behavior of the method in that way.
Perhaps tryOffer(): TryOfferResult or offerOrThrow(): ???. I'm not super happy with the naming of the later though as it implies that it throws if it wasn't able to offer.
I think send/offer is actually more confusing than send/trySend – the semantic distinction between "send" and "offer" is subtle, whereas trySend directly says what it does.
I also am not a fan of the param (although I don't feel as strongly). Code like offer("foo", true) is super unclear what true does and while it would be best practice to include an explicit param name here, not everyone's going to do that.
Too bad for those who use positional boolean parameters I'd say, that's a bad idea as soon as you have more than a parameter, and I'm hopeful Kotlin will ban it in source for non Java/JS symbols one day (maybe Kotlin 2.0?).
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.
Now, if trySend is to become a replacement to offer, its signature could be so to avoid that sealed class allocation:
fun trySend(
element: E,
throwIfClosed: Boolean = false
): Boolean
That way:
trySend(stuffFromCallback)) is the no-footgun option that will just return false is the channel is closedoffer with the ReplaceWith clause produces the following to make the behavior explicit trySend(element, throwIfClosed = true)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
}
Ah yes, I didn't have in mind sealed classes can have objects (even though I use it), which means no repeated allocation… That proposal works too.
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
Channelcan be anyThrowable, not necessarily aCancellationException, and a usual attempt tosendorofferover a "failed" channel would throw that exception.
That's one of the most beautiful lines of code I've ever seen.
@elizarov In your snippet, it doesn't exist (unresolved reference) since the signature of the lambda taken by isAcceptedOrIfClosed is of type () -> Boolean.
You need to get a reference to the exception that closed the channel for it to work, which of course cannot be put in an enum (but could be exposed in the SendChannel and be retrieved from the isAcceptedOrIfClosed function and put in block).
In your snippet,
itdoesn't exist (unresolved reference) since the signature of the lambda taken byisAcceptedOrIfClosedis of type() -> Boolean.
@LouisCAD Thanks a lot! A helpful reminder that I should not write code in GitHub but in the actual IDE. I'll update it.
We can also optimize the implementation a bit in terms of the number of classes. I don't think we need TrySendResult to publically expose its subclasses. I don't envision that people would actually write this kind of when(result) { .... } themselves. We can provide a smaller API surface with internally optimized implementation:
sealed class TrySendResult {
companion object {
val Accepted: TrySendResult
val Rejected: TrySendResult
fun Closed(cause: Throwable)
}
val isAccepted: Boolean
val isClosed: Boolean
val closeCauseOrNull: Throwable?
// no isRejected on purpose! Avoids missing closed case.
}
inline fun TrySendResult.isAcceptedOrIfClosed(onClosed: (Throwable) -> Boolean): Boolean = when {
isClosed -> onClosed(closeCauseOrNull!!)
isAccepted -> true
else -> false
}
It should suffice for all the typical use-cases:
trySend(x).isAcceptedOrIfClosed { throw it } // replacing offer, throw when closed
trySend(x).isClosed // manually check for closing to stop, ignore accepted/rejected
trySend(x).isAccepted // manually check for accepted send if rejection fallback is needed
Why you propose a Throwable instead of a CancellationException?
isClosed isn't a base property, it can be rewritten:
inline val TrySendResult.isClosed get() = closeCauseOrNull != null
Personal note: I hate if (condition) true else false :)
inline fun TrySendResult.isAcceptedOrIfClosed(onClosed: (CancellationException) -> Boolean): Boolean =
closeCauseOrNull?.let { onClosed(it) } ?: isAccepted
I don't envision that people would actually write this kind of
when(result) { .... }
Please consider this block:
if (trySend(x).isAcceptedOrIfClosed { throw it }) {
...
} else {
...
}
@elizarov Your last snippet doesn't compile, and I don't really understand it either.
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).
@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
closedCauseproperty onSendChannel, you can use it inisAcceptedOrIfClosedimplementation and keep using anenum(be it in screaming case or not).
Hm. That's an interesting idea!
Maybe make it more fluent?
trySend().ifClosed { throw it }.isAccepted
Here ifClosed returns itself (this: TrySendResult) after performing action
At least for me it is more comprehensible than isAcceptedOrIfClosed
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
Maybe make it more fluent?
trySend().ifClosed { throw it }.isAccepted
Here ifClosed returns itself (this: TrySendResult) after performing action
@pacher I like the idea too.
One thing to note about this is that it doesn't allow to return a boolean from the onClose block like proposed earlier in the thread. For instance, there would be no fluent equivalent to:
trySend(x).isAcceptedOrIfClosed { true }
That being said, I would argue that for this behaviour, the following may be clearer:
val sendResult = trySend(x)
if (sendResult.isAccepted || sendResult.isClosed) {
...
}
@joffrey-bion ifClosed is not supposed to return boolean, isAccepted does. ifClosed could be used to register some other action, not necessary throwing. But if you want a boolean, we can also have an extension function
fun TrySendResult.throwIfClosed(): Boolean = ... throws in case of error and returns accepted/rejected as boolean otherwise...
Then it can look like
val sentSuccessfully = trySend(x).throwIfClosed()
if (sentSuccessfully) {
...
}
This way throwing is explicit. User can either handle the 3 cases in when expression or call this extension function (if (trySend(x).throwIfClosed()) { .. }). Either way you have to deal with possible exception if you care about the result of trySend. If you don't, then no unexpected exceptions.
@pacher I was merely mentioning the initially proposed signature of isAcceptedOrIfClosed.
There were only examples with throw it in the body, but it did return a boolean.
Is this going to be addressed in kotlinx.coroutines 1.4.0?
This is a footgun that keeps appearing in third party libraries, adding more risks of app crash.
Latest example is Google Maps KTX for Android:
Yes, that's one of our goals.
Tho we can just add it right now due to compiler-related problems with inline-classes (with ETA around Kotlin 1.4.20).
We'll start prototyping the solution soon
I thought there was a solution using plain enums… there's an envisioned solution that'd be inline classes backed?
Should I try an implementation of trySend in a PR?
BTW, if having the solution for this issue in kotlinx.coroutines 1.4.0 is one of your goals, could it get the "For 1.4" label?
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.
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?
We still willing to fix it in 1.4.x
@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. :pleading_face:
@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.
Right, but my team says "no experimental ever", no matter what I say, thus i can't use this.
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.
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:
freeCompilerArgs so they don't see the warning._Disclaimer: These might all be bad ideas 😄_
Is the plan of resolution for this issue to evolve the API so it uses Kotlin 1.5 value classes with the @JvmInline optimization?
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:
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 themLock.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:
send, receive.trySend and tryReceive instead of offer and poll.catching. For now we only provide receiveCatching and onReceiveCatching (instead of previously internal receiveOrClosed)kotlin.Result (e.g. Deferred.awaitCatching) or domain-specific result class. For channels it is ChannelResult.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.
Most helpful comment
I've been bitten by that again just now (plus earlier)!
I know a bunch of code using
callbackFlow/channelFlowin the wild is unsafe as well becauseofferthrows as soon as the channel is closed, which happens before the block forawaitCloseis 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
awaitCloseis 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.