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

Propagate CoroutineContext in CoWebFilter #27522

Closed
be-hase opened this issue Oct 6, 2021 · 13 comments
Closed

Propagate CoroutineContext in CoWebFilter #27522

be-hase opened this issue Oct 6, 2021 · 13 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) theme: kotlin An issue related to Kotlin support type: enhancement A general enhancement
Milestone

Comments

@be-hase
Copy link

be-hase commented Oct 6, 2021

Motivation

There are cases where the coroutine dispatcher is changed when performing blocking processing.

@GetMapping("/sample")
suspend fun sample(): String {
    // do non-blocking call

    withContext(blockingDispatcher) {
        // do blocking call
    }

    // do non-blocking call

    ...
}

But now WebFlux uses Unconfined dispatcher.
https://github.com/spring-projects/spring-framework/blob/5.3.x/spring-core/src/main/java/org/springframework/core/CoroutinesUtils.java#L74

So once we change the dispatcher, subsequent processing will also be executed by that dispatcher.
For example, in the following example, it will be executed by blockingExecutor thread even after withContext.

@GetMapping("/test/hello1")
suspend fun hello1(): String {
    // reactor-http-nio-N thread
    log.info("thread={}", Thread.currentThread().name)

    withContext(blockingDispatcher) {
        // blockingExecutor-N thread
        log.info("thread={}", Thread.currentThread().name)

        // do blocking call
    }

    // blockingExecutor-N thread
    log.info("thread={}", Thread.currentThread().name)

    return "hello"
}

We can work around this issue by using the default dispatcher instead of the Unconfined dispatcher.

@GetMapping("/test/hello2")
suspend fun hello2(): String {
    return withContext(Dispatchers.Default) {
        // DefaultDispatcher-worker-N thread
        log.info("thread={}", Thread.currentThread().name)

        withContext(blockingDispatcher) {
            // blockingExecutor-N thread
            log.info("thread={}", Thread.currentThread().name)

            // do blocking call
        }

        // DefaultDispatcher-worker-N thread
        log.info("thread={}", Thread.currentThread().name)

        "hello"
    }
}

However, writing withContext(Dispatchers.Default) on all controller methods can be tedious.
So I want to be able to change the coroutine dispatcher used by WebFlux.

How

How about making it possible to change by registering a class such as CoroutinesDispatchersProvider in the bean?

interface CoroutinesDispatchersProvider {
    fun provide(request: ServerHttpRequest): CoroutineDispatcher
}

For example, a framework called armeria allows we to specify a dispatcher
https://armeria.dev/docs/server-annotated-service#coroutine-dispatcher

@be-hase be-hase changed the title Make coroutine Dispatcher changeable in WebFlux Make coroutine dispatcher changeable in WebFlux Oct 6, 2021
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Oct 6, 2021
@rstoyanchev rstoyanchev added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Nov 8, 2021
@be-hase
Copy link
Author

be-hase commented Dec 17, 2021

To give you another example, grpc-kotlin supports customizing coroutine context.
grpc/grpc-kotlin#66

Being able to customize the context makes it much easier to propagate MDCs and Brave's Tracing.
e.g. https://kotlin.github.io/kotlinx.coroutines/kotlinx-coroutines-slf4j/kotlinx.coroutines.slf4j/-m-d-c-context/index.html

Please consider it.

@sdeleuze sdeleuze added the theme: kotlin An issue related to Kotlin support label Jan 19, 2022
@zsiegel
Copy link

zsiegel commented Mar 1, 2022

This would be very helpful in regards to MDC context as mentioned above.

I would be happy to put together a PR if any of the team members can point me in a direction that might be suitable to put this configuration?

@be-hase
Copy link
Author

be-hase commented Mar 17, 2022

This would be very helpful in regards to MDC context as mentioned above.

Yes. This is very ver helpful for kotlin coroutine user.

@be-hase be-hase changed the title Make coroutine dispatcher changeable in WebFlux Make coroutine dispatcher(coroutine context) changeable in WebFlux Apr 8, 2022
@2hyjun
Copy link

2hyjun commented Oct 28, 2022

This feature has been implemented on this commit

Invoking suspend functions with customized CoroutineContext has been implemented, but there's no way to provide it currently.

@wplong11
Copy link
Contributor

wplong11 commented Oct 29, 2022

@2hyjun This feature is not implemented. That commit only added new invokeSuspendingFunction that accepts CoroutineContext. To implement this feature, InvocableHandlerMethod must be fixed to use new invokeSuspendingFunction and CoroutinesDispatchersProvider should be added.

Method method = getBridgedMethod();
if (KotlinDetector.isSuspendingFunction(method)) {
value = CoroutinesUtils.invokeSuspendingFunction(method, getBean(), args);
}
else {
value = method.invoke(getBean(), args);
}

@wplong11
Copy link
Contributor

wplong11 commented Oct 29, 2022

@rstoyanchev @be-hase Can we discuss about design of CoroutinesDispatchersProvider? If there is an agreement on the design, I am willing to write a PoC PR.

@be-hase
Copy link
Author

be-hase commented Nov 9, 2022

@wplong11
Sorry for the late reply.
of course! always ready to discuss :)

@sdeleuze sdeleuze self-assigned this Feb 8, 2023
@sdeleuze sdeleuze added this to the 6.0.6 milestone Feb 8, 2023
@sdeleuze sdeleuze added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Feb 8, 2023
@sdeleuze sdeleuze modified the milestones: 6.0.6, 6.1.x Feb 14, 2023
@efemoney
Copy link

efemoney commented Jul 14, 2023

It should be a HandlerMethodCoroutineContextProvider and not just a dispatcher provider. To implement things like CoroutineNameing endpoint calls, adding custom context elements and also pave the way for 3p to integrate

@sdeleuze sdeleuze modified the milestones: 6.1.x, 6.1.0-RC1 Aug 28, 2023
@sdeleuze
Copy link
Contributor

Let's try to see if we can leverage CoWebFilter to support this use case by taking inspiration of #26977 (comment).

@be-hase
Copy link
Author

be-hase commented Sep 7, 2023

Thank you.
But you must be using router function style, right...? 😢

@sdeleuze sdeleuze changed the title Make coroutine dispatcher(coroutine context) changeable in WebFlux Propagate CoroutineContext in CoWebFilter Sep 7, 2023
@sdeleuze
Copy link
Contributor

sdeleuze commented Sep 7, 2023

CoWebFilter now allows to customize, potentially dynamically based on the request, the CoroutinesContext.

sdeleuze added a commit to sdeleuze/spring-framework that referenced this issue Sep 12, 2023
To avoid compilation errors in Eclipse which does
not support Java code dependency on Kotlin code.

See spring-projectsgh-27522
sdeleuze added a commit to sdeleuze/spring-framework that referenced this issue Sep 12, 2023
@tekener
Copy link

tekener commented Sep 25, 2023

Looks like with this implementation always the coroutine context of the last filter is winning.
I was expecting that you could add more context information to the context created by the previous filter.

@sdeleuze
Copy link
Contributor

sdeleuze commented Oct 8, 2023

Please create a new dedicated issue with a reproducer if you think there is something to refine/fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) theme: kotlin An issue related to Kotlin support type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

9 participants