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

Consistent handling of coroutine context #32578

Open
ilya40umov opened this issue Apr 5, 2024 · 6 comments
Open

Consistent handling of coroutine context #32578

ilya40umov opened this issue Apr 5, 2024 · 6 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged or decided on theme: kotlin An issue related to Kotlin support

Comments

@ilya40umov
Copy link
Contributor

While refactoring the filter chain used in one of our services, which is based on Kotlin, Spring Boot, WebFlux, coRouter & coroutines, I've run in the following scenario:

  • observationRegistry.asContextElement() needs to be added early on to the context, so that the observation from http request is correctly propagated
  • multiple separate concerns, such as adding trace baggage, logging incoming request etc., need to be implemented as individual filters
  • since we are using coRouter, some of the filters need to be only applied to some of the routes defined in the DSL

Here are the facilities I'm aware of / was able to find, which seem to be relevant for the problem at hand:

  • CoWebFilter
  • fun filter(filterFunction: suspend (ServerRequest, suspend (ServerRequest) -> ServerResponse) -> ServerResponse) in CoRouterFunctionDsl
  • fun context(provider: suspend (ServerRequest) -> CoroutineContext) in CoRouterFunctionDsl

Now, here are the problems I've run into:

  • Building a chain of CoWebFilters would require making them all aware of which particular EPs to wrap and which to pass on
  • Using fun filter(filterFunction) from CoRouterFunctionDsl allows to apply these in some parts of coRouter, but these filter functions aren't picking up the context that CoWebFilter may have left in COROUTINE_CONTEXT_ATTRIBUTE.
  • Additionally, all filters created by fun filter(filterFunction) will not inherit context from one another and aren't able to modify the context that the actual handler will use
  • If fun context(provider) is used, it's executed multiple times for 1 request. I.e. it will be called to create a context for each filter, and then for the corresponding handler.

In the end, I've ended up with the following "magical" implementation:

coRouter {
    context { request ->
        if (CoWebFilter.COROUTINE_CONTEXT_ATTRIBUTE !in request.attributes()) {
            request.attributes()[CoWebFilter.COROUTINE_CONTEXT_ATTRIBUTE] =
                Dispatchers.Unconfined + observationRegistry.asContextElement()
        }
        request.attributes()[CoWebFilter.COROUTINE_CONTEXT_ATTRIBUTE] as CoroutineContext
    }
    filter(baggageAddingFilter)
    filter(requestLoggingFilter)
    routes()
}

which is at least able to meet our current needs, but it still has a problem that filters added this way would only be able to modify coroutineContext of one another by modifying request.attributes()[CoWebFilter.COROUTINE_CONTEXT_ATTRIBUTE] explicitly.

IMO, Spring Framework could:

  • provide more consistent support for persisting/inheriting coroutine context between parts of the execution chain.
  • potentially look into adding a facility similar to fun context(provider) of coRouter that would be executed early on and provide the context for the first `CoWebFilter in the chain
  • reevaluate how many times fun context(provider) should be executed by coRouter during handling of a single request (e.g. it could be for example used as a fallback to provide coroutineContext once, if by the time execution goes into coRouter code there was no CoWebFilter invoked).

Tested on: Spring Boot 3.2.4 / Spring 6.1.5

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Apr 5, 2024
@jhoeller jhoeller added in: web Issues in web modules (web, webmvc, webflux, websocket) theme: kotlin An issue related to Kotlin support labels Apr 5, 2024
@sdeleuze
Copy link
Contributor

sdeleuze commented Sep 5, 2024

Hi, thanks for the detailed feedback and sorry for the delay, I think there is room for refinements indeed. I think we need to discuss to try to identify more focused individual refinements. Any chance you could provide focused repro(s) as a link to a repositiory or an attached project for the individual issues you are raising here?

I am wondering if adding a ServerRequest extensions like request.coroutineContext() could help combined with other context propagation refinements, any thoughts?

Building a chain of CoWebFilters would require making them all aware of which particular EPs to wrap and which to pass on

What do you mean by "EPs"?

@sdeleuze sdeleuze added the status: waiting-for-feedback We need additional information before we can continue label Sep 5, 2024
@spring-projects-issues
Copy link
Collaborator

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

@spring-projects-issues spring-projects-issues added the status: feedback-reminder We've sent a reminder that we need additional information before we can continue label Sep 12, 2024
@ilya40umov
Copy link
Contributor Author

ilya40umov commented Sep 12, 2024

Alright, so as for the reproducer I have created something that you can refer to here:

  • Router.kt - showing a couple of versions based on coRouter DSL and its filters
    • V1 is a naive implementation and it does not work (as it's trying to rely on "withContext" propagation between the filters etc.)
    • V2 is a working implementation that is based on CoWebFilter.COROUTINE_CONTEXT_ATTRIBUTE attribute, but it's also super hacky

What do you mean by "EPs"?

Ah, sorry, EPs would stand for "endpoints" in this case.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue status: feedback-reminder We've sent a reminder that we need additional information before we can continue labels Sep 12, 2024
@ilya40umov
Copy link
Contributor Author

ilya40umov commented Sep 12, 2024

Essentially, I would expect that V1 implementations would work out of the box:

when defined in the router like this.

However, in reality to achieve context propagation between the filters I had to rewrite them like follows:

And additionally set up a context provider on the coRouter level. And this context provider is basically called for each filter in the chain separately.

@sdeleuze
Copy link
Contributor

Thanks for your detailed feedback, I will let you know when I have clarified what we can/can't do and when.

@ilya40umov
Copy link
Contributor Author

Ran into another (mostly unrelated) problem with the coroutine context propagation and raised the following PR: #33548

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) status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged or decided on theme: kotlin An issue related to Kotlin support
Projects
None yet
Development

No branches or pull requests

4 participants