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

Consistently handle exceptions in reactive streams #2646

Merged
merged 7 commits into from Apr 20, 2021

Conversation

dkhalanskyjb
Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb commented Apr 13, 2021

  • Fixed PublisherCoroutine, rxObservable, and
    Flow.toPublisher ignoring cancellations.
  • Fatal exceptions are not treated in a special manner by us
    anymore. Instead, we follow the requirement in the reactive
    streams specification that, in case some method of Subscriber
    throws, that subscriber MUST be considered canceled, and the
    exception MUST be reported in someplace other than onError.
  • Fixed trySend sometimes throwing in PublisherCoroutine and
    rxObservable.
  • When an exception happens inside a cancellation handler, we now
    consistently throw the original exception passed to the handler,
    with the new exception added as suppressed.
  • Fixed PublisherCoroutine and rxObservable claiming that the
    channel is not closed for send for some time after close() has
    finished.
  • Fixed publishers sometimes signalling onComplete() after
    cancellation even though their streams are not finite.

Fixes #2173

@dkhalanskyjb
Copy link
Collaborator Author

I will be adding some tests to this PR but would appreciate some feedback on the implementation in the meantime.

@dkhalanskyjb dkhalanskyjb force-pushed the fix-publishing-ignoring-cancellations branch from f7c53ae to ed1cf6d Compare April 13, 2021 12:05
Copy link
Member

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's amazing and thoughtful work from you, thanks!

reactive/kotlinx-coroutines-reactive/src/Publish.kt Outdated Show resolved Hide resolved
@dkhalanskyjb dkhalanskyjb force-pushed the fix-publishing-ignoring-cancellations branch from ce6cc5f to bee8e90 Compare April 14, 2021 12:43
* Fixed `PublisherCoroutine` and `rxObservable` ignoring
  cancellations.
* Fatal exceptions are not treated in a special manner by us
  anymore. Instead, we follow the requirement in the reactive
  streams specification that, in case some method of `Subscriber`
  throws, that subscriber MUST be considered cancelled, and the
  exception MUST be reported in some place other than `onError`.
* Fixed `trySend` sometimes throwing in `PublisherCoroutine` and
  `rxObservable`.
* When an exception happens inside a cancellation handler, we now
  consistently throw the original exception passed to the handler,
  with the new exception added as suppressed.
@dkhalanskyjb dkhalanskyjb force-pushed the fix-publishing-ignoring-cancellations branch from bee8e90 to 8f9c70d Compare April 16, 2021 13:52
* Fixed `doLockedNext` not releasing the lock in
  `PublisherCoroutine` if `null` is emitted
* Fixed `flux`, `publish`, `rxObservable` and `rxFlowable`
  incorrectly reporting `isClosedForSend == true` just after
  closing the channel
Copy link
Member

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to go

subscriber.onComplete()
} else {
@Suppress("INVISIBLE_MEMBER")
val unwrappedCause = unwrap(cause)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#2551 filed a use-case

@dkhalanskyjb dkhalanskyjb merged commit 8bb5210 into develop Apr 20, 2021
@dkhalanskyjb dkhalanskyjb deleted the fix-publishing-ignoring-cancellations branch April 20, 2021 08:21
pablobaxter pushed a commit to pablobaxter/kotlinx.coroutines that referenced this pull request Sep 14, 2022
* Fixed `PublisherCoroutine`, `rxObservable`, and
  `Flow.toPublisher` ignoring cancellations.
* Fatal exceptions are not treated in a special manner by us
  anymore. Instead, we follow the requirement in the reactive
  streams specification that, in case some method of `Subscriber`
  throws, that subscriber MUST be considered canceled, and the
  exception MUST be reported in someplace other than `onError`.
* Fixed `trySend` sometimes throwing in `PublisherCoroutine` and
  `rxObservable`.
* When an exception happens inside a cancellation handler, we now
  consistently throw the original exception passed to the handler,
  with the new exception added as suppressed.
* Fixed `PublisherCoroutine` and `rxObservable` claiming that the
  channel is not closed for send for some time after `close()` has
  finished.
* Fixed publishers sometimes signalling `onComplete()` after
  cancellation even though their streams are not finite.

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

Successfully merging this pull request may close these issues.

None yet

2 participants