Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CancellableContinuation.invokeOnCancellation cause is inconsistent and breaks its own contract #2089

Open
qwwdfsad opened this issue Jun 14, 2020 · 4 comments

Comments

@qwwdfsad
Copy link
Member

Steps to reproduce:

  1. This snippet prints null
suspendCancellableCoroutine<Unit> { c ->
    c.invokeOnCancellation {
        println(it) 
    }
    c.cancel()
}
  1. Semantically the same snippet prints CancellationException:
suspendCancellableCoroutine<Unit> { c ->
    c.cancel()
    c.invokeOnCancellation {
        println(it) 
    }
}

CompletionHandler contract:

 * The meaning of `cause` that is passed to the handler:
 * * Cause is `null` when the job has completed normally.
 * * Cause is an instance of [CancellationException] when the job was cancelled _normally_.
 *   **It should not be treated as an error**. In particular, it should not be reported to error logs.
 * * Otherwise, the job had _failed_.
@qwwdfsad
Copy link
Member Author

This behaviour seems to be the legacy from the times when CancellableContinuation was an instance of Job (pre 0.22.x versions) and we preserved CompletionHandler ABI.
Tho for CancellableContinuation that does not have invokeOnCompletion the underlying cause is never null.

@qwwdfsad
Copy link
Member Author

qwwdfsad commented Jun 14, 2020

One more inconsistency:
1)

suspendCancellableCoroutine<Unit> { c ->
    c.invokeOnCancellation { 
        println("Invoked") // Printed
    }
     c.invokeOnCancellation { expectUnreached() } // Triggers ISE
}
suspendCancellableCoroutine<Unit> { c ->
    c.invokeOnCancellation { 
        println("Invoked")  // Not printed
    }
    c.resumeWith(Result.failure(IllegalStateException()))
}

@qwwdfsad qwwdfsad changed the title CacnellableContinuation.invokeOnCancellation cause is inconsistent and breaks its own contract CancellableContinuation.invokeOnCancellation cause is inconsistent and breaks its own contract Jun 15, 2020
@elizarov
Copy link
Contributor

As a side note. I've rechecked both in resource-cancel branch (which reworks a significant chunk of CacncellableContinuationImpl machinery):

  1. The first inconsistency with cause is still present.

  2. The second inconsistency with throwing from invokeOnCancellation is gone, but what should happen if invokeOnCancellation throws an exception is still an open design issue in PR Breaking: Get rid of atomic cancellation and provide a replacement #1937 and that's the reason why it is still a draft.

@dkhalanskyjb
Copy link
Collaborator

Both inconsistencies are still there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants