Don’t force a suspend #844
Conversation
|
Requires more than a quick perusal so I'll look later. |
| * Immediate result from invoking a suspending function, that may include either a value (if the | ||
| * call never suspended) or a cancel callback (if it did suspend). | ||
| */ | ||
| internal class SuspendingResult<T>( |
There was a problem hiding this comment.
Considering for follow-up . . .
We have a bunch of result types:
- kotlin.Result - success or failure
- app.cash.zipline.CallResult - success or failure, encoded JSON, service names
- app.cash.zipline.internal.bridge.SuspendingResult - success or failure or callback
- app.cash.zipline.internal.bridge.EncodedSuspendingResult - success or failure or callback, encoded JSON, service names
We also have serializers for kotlin.Result and SuspendingResult.
Some options to reduce how much code we need:
- Combine the two serializers to save a bit of code. Rename SuspendingResult to InternalResult for symmetry with InternalCall.
- Change EncodedSuspendingResult to have a
kotlin.Resultas a field
|
|
||
| val cancelCallback = object : CancelCallback, HasPassByReferenceName { | ||
| override var passbyReferenceName: String? = null | ||
| deferred.invokeOnCompletion { |
There was a problem hiding this comment.
I'm a bit worried about the Retrofit case here: where isActive returns true but by the time you get to this code the async has completed and thus this lambda will be invoked synchronously (I'm assuming that's the behavior).
It would mean that we invoke the suspendCallback below synchronously from the stackframe which invoked the suspend function (and before we allow it to return a value). The normal coroutine machinery defends against this (which is what was happening in Retrofit).
In our case, to defend against it, I think we need something like the following:
var scheduleCallback = true
deferred.invokeOnCompletion {
// ..
if (scheduleCallback) {
ziplineDispatcher.schedule({ /* callback logic */ })
} else {
// callback logic
}
}
scheduleCallback = falseThere was a problem hiding this comment.
It’s single-threaded, so we don‘t need to worry about that.
There was a problem hiding this comment.
As in, the async is running on the same dispatcher as this block? That's good.
| val kotlinResult = when { | ||
| failure != null -> Result.failure(failure) | ||
| else -> deferred.getCompleted() | ||
| } | ||
|
|
||
| override fun toString() = "CancelCallback/$internalCall" | ||
| } | ||
| job.invokeOnCompletion { | ||
| val name = cancelCallback.passbyReferenceName | ||
| if (name != null) endpoint.remove(name) | ||
| } | ||
| val suspendingResult = when { | ||
| kotlinResult.isFailure -> SuspendingResult<Unit>(failure = kotlinResult.exceptionOrNull()!!) | ||
| else -> SuspendingResult(success = kotlinResult.getOrNull()) | ||
| } |
There was a problem hiding this comment.
These two when's need merged or something. You create the kotlin.Result and then unpack it just to match on it, force unwrap, and then re-wrap it.
There was a problem hiding this comment.
Ooooh it’s weirder than that. Added this comment:
// Convert our Deferred<Result<T>> into a Result<T>, which will be failure if _either_ the
// Deferred failed or if it contains a failure Result.
There was a problem hiding this comment.
(Is this possible??)
| @@ -214,6 +214,7 @@ internal class RealCallSerializer( | |||
| return object : SuspendingZiplineFunction<T>( | |||
| name = functionName, | |||
| argSerializers = listOf(), | |||
| resultSerializer = Int.serializer(), // Placeholder; we're only encoding failures. | |||
There was a problem hiding this comment.
There's a NothingSerializer coming in the next version of kotlinx.coroutines.
zipline/src/commonMain/kotlin/app/cash/zipline/internal/bridge/calls.kt
Outdated
Show resolved
Hide resolved
| /** The function suspended. Only non-null for suspend calls. */ | ||
| val cancelCallback: CancelCallback? = null, | ||
|
|
||
| /** The function failed without suspending. */ | ||
| val failure: Throwable? = null, | ||
|
|
||
| /** The function succeeded without suspending. */ | ||
| val success: T? = null, |
There was a problem hiding this comment.
Do you think we need an init { } which enforces the invariant that one (and only one) is set? Deserialization below could create an instance with all nulls, for example.
There was a problem hiding this comment.
Good idea. Done.
…/calls.kt Co-authored-by: Jake Wharton <jw@squareup.com>
|
Ugh, I’ve got some unwanted concurrency in the tests. |
|
Okay so eventually I decided to run After scouring Stackoverflow for that error I came across this question, which recommended adding It’s Quite Unfortunate that our stacks are deep enough to cause a JVM process crash. |
This change is implementation-details only and doesn't change the data passed between caller and target. Follow-up to #844
This change is implementation-details only and doesn't change the data passed between caller and target. Follow-up to #844
Closes: #826