Skip to content
This repository was archived by the owner on Mar 2, 2022. It is now read-only.

Fix conversion to Scala Future #36

Merged
merged 2 commits into from
Oct 10, 2019
Merged

Fix conversion to Scala Future #36

merged 2 commits into from
Oct 10, 2019

Conversation

chetanmeh
Copy link
Contributor

Current implementation for future conversion does not work for cases where Mono was completed with a null/void result.

https://github.com/reactor/reactor-scala-extensions/blob/8ecf0d5fd4fad34b8886499aa03993045e00ce65/src/main/scala/reactor/core/scala/publisher/SMono.scala#L1300-L1308

Here due to use of Option with value the promise would not be completed if the value is null. On the other hand the way Scala Future compat logic handles this by passing a BiConsumer which considers the throwable == null case as Success else a Failure

class P[T](val wrapped: CompletionStage[T]) extends DefaultPromise[T] with BiConsumer[T, Throwable] {
    override def accept(v: T, e: Throwable): Unit = {
      if (e == null) complete(Success(v))
      else complete(Failure(e))
    }
  }

As a result this PR fixes the current issue by completing promise with success if throwable is null

Current implementation for future conversion does not work for cases where Mono was completed with a null/void result. Fixed that by completing promise with success if throwable is null
@pivotal-issuemaster
Copy link

@chetanmeh Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster
Copy link

@chetanmeh Thank you for signing the Contributor License Agreement!

@sinwe
Copy link
Contributor

sinwe commented Oct 10, 2019

@chetanmeh Thank you for your PR. I've put up a comment. If you could address it quickly, I can merge it to master and cut a version

@chetanmeh
Copy link
Contributor Author

I've put up a comment. If you could address it quickly, I can merge it to master and cut a version

@sinwe I do not see any comment here. May be you missed submitting the review?

@sinwe
Copy link
Contributor

sinwe commented Oct 10, 2019

I've put up a comment. If you could address it quickly, I can merge it to master and cut a version

@sinwe I do not see any comment here. May be you missed submitting the review?

@chetanmeh Sorry, forgotten to submit the review :). Now submitted already

@sinwe sinwe merged commit fe1e962 into spring-attic:master Oct 10, 2019
@sinwe
Copy link
Contributor

sinwe commented Oct 10, 2019

@chetanmeh I've merged into master. Unless there is anymore PR in the next 1-2 days, I'll cut a release version.

@chetanmeh
Copy link
Contributor Author

Thanks @sinwe for the prompt review and merge.

@sinwe
Copy link
Contributor

sinwe commented Oct 18, 2019

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants