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

Coroutine context propagation for Reactor to coroutines API migration #1377

Merged
merged 7 commits into from Aug 9, 2019

Conversation

SokolovaMaria
Copy link
Contributor

@SokolovaMaria SokolovaMaria commented Jul 24, 2019

We've supported the following cases of the coroutine context propagation for Reactor to coroutines API migration:

  • kotlinx.coroutines.reactive contains extension functions for (Mono/Flux) to suspending function: suspend fun <T> Publisher<T>.await*(): T?. Coroutine context of these await functions is propagated into Publisher's context. Consider the use case:
suspend fun foo() {
    ...
    bar().awaitFirst()
}

fun bar(): Mono<T> { ... } 
  • The same idea introduced for migration from Reactor's Publisher to Flow: suspending collect call propagates it's coroutine context to the source Publisher context.
// function that uses new coroutines Flow API
suspend fun bar(): Flow<T> {
     return oldBar().asFlow()
}
// legacy function that returns Publisher
fun oldBar(): Flux<T> {...}

suspend fun foo() {
     bar().collect() 
}

This propagation is implemented the following way:

  • We provide ContextInjector service API that contains the only function to inject the given coroutine context into the Publisher:
public interface ContextInjector {
    public fun <T> injectCoroutineContext(publisher: Publisher<T>, coroutineContext: CoroutineContext): Publisher<T>
}
  • reactor module contains the implementation of this interface (ReactorContextInjector) that enriches Reactor's Mono/Flux contexts with values from coroutineContext[ReactorContext], instances of other Publisher types are returned unchanged.

  • Publisher<T>.await*() functions search for the available ContextInjector services (if reactor module is included - ReactorContextInjector is available, otherwise no services found) and enrich Publisher's context if possible.
    PublisherAsFlow injects the coroutine context of collect call the same way.

Available services are searched once, if reactor module is not included - the list of services is empty, so no overhead added.

@SokolovaMaria SokolovaMaria changed the title Propagation of the coroutine context of await* calls into Mono/Flux builders Propagation of the coroutine context of await* calls into Mono/Flux builders Jul 24, 2019
@qwwdfsad qwwdfsad self-requested a review July 24, 2019 15:53
@qwwdfsad qwwdfsad added this to the 1.3 milestone Jul 24, 2019
@SokolovaMaria SokolovaMaria changed the title Propagation of the coroutine context of await* calls into Mono/Flux builders Coroutine context propagation for Reactor to coroutines API migration Jul 26, 2019
* Propagation of the coroutine context of await calls into
  Mono/Flux builder
* Publisher.asFlow propagates coroutine context from `collect`
  call to the Publisher
* Flow.asFlux transform

Fixes #284
@elizarov
Copy link
Contributor

I've resolved some of the issues, squashed commits, dropped .flow package from reactive modules, remove @JvmName("from") from conversion methods (as it is not used in other conversion methods of reactive module), but moved them to a more future-proof FlowKt JVM class.

Force-pushed

@elizarov elizarov requested a review from qwwdfsad July 30, 2019 09:25
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.

Integration in produceImpl is missing

@elizarov

This comment has been minimized.

@elizarov elizarov requested a review from qwwdfsad July 31, 2019 09:26
…from onComplete and onError, make context injector R8-friendly, do not use flowOn for empty context
@qwwdfsad qwwdfsad requested a review from elizarov August 6, 2019 09:50
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.

LGTM

@qwwdfsad qwwdfsad requested a review from elizarov August 6, 2019 11:43
@qwwdfsad qwwdfsad requested a review from elizarov August 8, 2019 11:59
Copy link
Contributor

@elizarov elizarov left a comment

Choose a reason for hiding this comment

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

Look great now.

@qwwdfsad qwwdfsad merged commit 1dcfd97 into develop Aug 9, 2019
@qwwdfsad qwwdfsad deleted the reactor-context branch August 9, 2019 14:35
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

3 participants