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

Mechanism to access request bound objects in WebClient filter in servlet env #25710

Closed
ttddyy opened this issue Sep 5, 2020 · 26 comments
Closed
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: documentation A documentation task type: task A general task
Milestone

Comments

@ttddyy
Copy link
Contributor

ttddyy commented Sep 5, 2020

In servlet environment, when migrating RestTemplate to WebClient, one of the challenges people face is to access http request bound objects within WebClient filters(ExchangeFilterFunction).
I think this is an area currently lacking a support.

For example, SecurityContext, Locale/LocaleContext(LocaleContextHolder), HttpServletRequest(RequestContextHolder) and any values associated to the request(ThreadLocal) cannot easily be retrieved in ExchangeFilterFunction.

I think common solution for this is to use Hooks to populate the values to subscriber's context.

For example, in Spring Security, here defines and registers a hook that populates SecurityContext.
Since this mechanism also populates HttpServletRequest and HttpServletResponse, I leverage it in my exchange filter functions to retrieve request bound values.
I also have similar logic for MDC and LocaleContext.

I think this mechanism should be supported in Spring Framework itself; so that, all downstream libraries and application can leverage it.

For implementation perspective, for example, define a class ReactorContextAttribute which is a map kept in thread local. Then, framework guarantees to populate this map in subscriber's context. So that, users or downstream libraries can simply populate this map in request thread, then retrieves values from the map in subscriber context.
In addition, FrameworkServlet/DispatcherServlet resolved value/context, such as LocaleContext, can be placed in this map to make them accessible in exchange filter functions.

If such mechanism exists, for example, Spring Security can simply add a logic to populate SecurityContext.

I think this is a big missing piece to use WebClient in servlet environment along with supporting MDC.

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Sep 11, 2020

@ttddyy, I'm wondering if you've seen the option to add attributes which a filter can then check via ClientRequest:

client.get().uri("https://example.org/")
        .attribute("myAttribute", "...")
        .retrieve()

This can be set up at the WebClient builder level to automate the extraction of ThreadLocal context:

WebClient client = this.builder
		.defaultRequest(spec -> spec.attribute("foo", FooContext.get()))
		.filter(filter)
		.build();

Spring Security uses this and its filter exposes a Consumer that can be used to configure a WebClient builder to extract and pass the information the filter needs.

I believe the mechanism is there for applications to extract and pass exactly what is needed. The mechanics are straight forward and I don't see much value and adding further conventions around that.

We can certainly do better to address this need more specifically in the documentation. There is a mention of the attributes but nothing to address the use case and to tie all of the above together.

@rstoyanchev rstoyanchev added the status: waiting-for-feedback We need additional information before we can continue label Sep 11, 2020
@ttddyy
Copy link
Contributor Author

ttddyy commented Sep 11, 2020

@rstoyanchev Oh, I wasn't aware of attributes method. Thanks.
Certainly it would be nice to mention ThreadLocal in documentation for attribute method usage.

I have simulated couple of our usecases trying with attributes in servlet environment with WebClient.
This serves most of the cases since I can assign values to attributes from Threadlocal at the time when request is made and on the thread that is making a request.

However, I found one case hard to do with attributes, which is nesting remote calls.
For example, make a first remote call, then based on the response, create another remote call.

Sample code:

ExchangeFilterFunction filter = (request, next) -> {
  log.info("filter map=" + request.attributes());
  return next.exchange(request);
};

WebClient webClient = WebClient.builder().filter(filter).defaultRequest(spec -> {
  spec.attribute("bar", "BAR");
  spec.attribute("tid", Thread.currentThread().getId());
  spec.attribute("tname", Thread.currentThread().getName());
}).build();

webClient.get().uri("http://service1.com")  // first call
    .attribute("foo", "FOO")
    .retrieve()
    .bodyToMono(String.class)
    .flatMap(str -> {
      return webClient.get().uri("http://service2.com")  // second call
          .retrieve()
          .bodyToMono(String.class);
    })
    .subscribeOn(Schedulers.boundedElastic())  // on different thread...
    .block();

When making the first call, attributes tid and tname are main thread, but for the second call, the thread is on elastic thread; so cannot retrieve thread local values that are bound to the caller.

In the real environment, these two calls are service-to-service calls and when I call another service, I need to propagate some values stored in caller's thread local. (very much similar to ServletBearerExchangeFilterFunction in Spring Security)

Workaround can be, when constructing the second call, explicitly add attribute method and pass a value that the filter is looking for. However, it is invasive to application code and would like to handle it in filter or construction of webclient in library.

Currently, I use subscriber context approach as mentioned in the description. (reusing SecurityReactorContextSubscriberRegistrar from Spring Security)

Also, interesting thing is that the ServletOAuth2AuthorizedClientExchangeFilterFunction (pointed in your comment) uses attributes but also it is getting attribute values(request, response, and auth) from subscriber context which is populated by the lifter hook (SecurityReactorContextSubscriberRegistrar).

So, attributes works for single remote call cases, but when multiple calls on different threads are involved, I think above mentioned Hook approach may be in need.

@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 labels Sep 11, 2020
@ttddyy
Copy link
Contributor Author

ttddyy commented Sep 13, 2020

If WebClient.Builder could setup its subscriber context(defaultContext method below), maybe this would work.

FooContext.set("main-thread");  // setting to threadlocal

// a filter uses "foo" from subscriber context
ExchangeFilterFunction filter = (request, next) ->
    Mono.subscriberContext()
        .map(context -> (String) context.get("foo"))
        .map(foo -> ClientRequest.from(request).header("foo", foo).build())
        .flatMap(next::exchange);


WebClient webClient = WebClient.builder()
        .filter(filter)
        // populate subscriber context
        .defaultContext(context -> context.hasKey("foo") ? context : context.put("foo", FooContext.get()))
        .build();

webClient.get()
        .uri("http://foo.com")
        .retrieve()
        .bodyToMono(String.class)
        .flatMap(body ->
                webClient.get()
                        .uri("http://bar.com")
                        .retrieve()
                        .bodyToMono(String.class)
        )
        .subscribeOn(Schedulers.boundedElastic())  // on different thread...
        ....
;

// both calls will have header "foo=main-thread"

When nested WebClient is called, the context key "foo" has already populated by outer WebClient, which used the original caller's thread.

Not sure it is even possible, but an idea.

@rstoyanchev rstoyanchev self-assigned this Sep 14, 2020
@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) type: documentation A documentation task type: enhancement A general enhancement and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged or decided on labels Sep 14, 2020
@rstoyanchev rstoyanchev added this to the 5.3 RC2 milestone Sep 14, 2020
@rstoyanchev
Copy link
Contributor

Thanks for the feedback. I've scheduled this since we'll update the docs in the very least.

Indeed the use of attributes does not propagate to nested requests. We could explore a defaultContext next to defaultRequest in WebClient.Builder. Another thought would be to add an option to have ClientRequest attributes be saved under some well known key in the Reactor context. That would also allows us to propagate attributes from parent to nested requests, so you could access them conveniently.

@ttddyy
Copy link
Contributor Author

ttddyy commented Sep 15, 2020

add an option to have ClientRequest attributes be saved under some well known key in the Reactor context.

This sounds a nice solution.

It may also be good to auto attach servlet request, etc to the reactor context if in servlet environment; So that, ExchangeFilterFunction can have consistent way to retrieve servlet request and values associated to the attributes.
This gives benefit to library writers since everything concludes in ExchangeFilterFunction, maybe in combination with servlet Filter to populates values to request attributes, instead of interleaving WebClient.Builder or WebClient#attributes at WebClient build time.

@rstoyanchev
Copy link
Contributor

It may also be good to auto attach servlet request

We can aim to make it as easy as possible but we can't make the assumption that it is always needed.

@ttddyy
Copy link
Contributor Author

ttddyy commented Sep 17, 2020

We can aim to make it as easy as possible but we can't make the assumption that it is always needed.

Ok, thanks.
At least having mechanism to pass values to nested calls is really helpful. Then, WebClientCustomizer can handle population part in boot application.

@rstoyanchev rstoyanchev modified the milestones: 5.3 RC2, 5.3 GA Oct 12, 2020
@rstoyanchev
Copy link
Contributor

On further thought a general solution for inheriting attributes from outer requests is challenging without control over how to merge outer and inner attributes. That means attributes remain scoped to a specific request only.

For anything more global it would have to be the Reactor context. This is already be possible and I don't see anything further we can do in the framework. For WebClientCustomizer one could insert a filter in order to update the context for every request. I'll experiment to confirm and focus on documentation updates around this scenario.

@rstoyanchev rstoyanchev modified the milestones: 5.3 GA, 5.3.1 Oct 27, 2020
@rstoyanchev rstoyanchev removed the type: documentation A documentation task label Nov 9, 2020
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Nov 9, 2020

I ended adding a context method, next to the attribute method for a WebClient request. Both can also be used in combination with defaultRequest at the WebClient.Builder level so they can be applied globally. I've improved the docs as well. I think this should meet you requirements but please take a look.

Thanks for creating this request!

@robotmrv
Copy link

it seem there is a problem with current approach
please see
79f79e9#r44087225

@rstoyanchev
Copy link
Contributor

@robotmrv you're right, the context can only propagate from downstream to upstream which means the new context method can only apply to the current request but that's no different from what you can already do with request attributes, except perhaps for any extra operations initiated within the filter chain that can benefit from the context.

This brings it back to where it's up to the (Spring MVC) application to do something like below to ensure the context is available throughout the entire request handling chain for a given request:

@GetMapping(...)
public String handle(...) {
	return client.get().uri("https://example.org/")
		.retrieve()
		.bodyToMono(String.class)
		.flatMap(body -> {
			// perform nested request...
		})
		.contextWrite(context -> ...);
}

I'm re-opening to consider further steps, possibly deprecating the new method, and documenting the above as the approach for Spring MVC applications to follow.

@rstoyanchev rstoyanchev reopened this Nov 11, 2020
@rstoyanchev rstoyanchev modified the milestones: 5.3.1, 5.3.2 Nov 11, 2020
@rstoyanchev rstoyanchev added type: documentation A documentation task type: task A general task and removed type: enhancement A general enhancement labels Nov 11, 2020
@robotmrv
Copy link

@rstoyanchev
BTW
I found that people are used to ThreadLocals and have problems in switching to reactive approach (reactive Web MVC / Webflux) because ThreadLocal context are not propagated automatically.
Because of this there are cases when people switching to other reactive frameworks which provide such functionality.

Are there any plans to provide context propagation functionality in spring-framework (at least in WebMVC) similar to microprofile context propagation / smallrye context propagation ?

@ttddyy
Copy link
Contributor Author

ttddyy commented Nov 12, 2020

Hi,

Can the context method be equal to transform with custom Subscriber that propagates current context?

Something like:

static class ContextPropagatingSubscriber<T> implements CoreSubscriber<T> {

  private final CoreSubscriber<T> delegate;

  private final Context context;

  public ContextPropagatingSubscriber(CoreSubscriber<T> delegate) {
    this.delegate = delegate;
    this.context = this.delegate.currentContext();
  }

  @Override
  public Context currentContext() {
    return this.context;
  }

  @Override
  public void onSubscribe(Subscription s) {
    this.delegate.onSubscribe(s);
  }

  @Override
  public void onNext(T t) {
    this.delegate.onNext(t);
  }

  @Override
  public void onError(Throwable t) {
    this.delegate.onError(t);
  }

  @Override
  public void onComplete() {
    this.delegate.onComplete();
  }

}
Function<? super Mono<String>, ? extends Publisher<Object>> transformer =
    Operators.liftPublisher((pub, subscriber) ->
        new ContextPropagatingSubscriber(subscriber));

webClient.get().uri("http://example.com")
    .retrieve()
    .bodyToMono(String.class)
    .publishOn(Schedulers.boundedElastic())
    .flatMap(str -> {
      return Mono.deferContextual(view -> {
        return webClient.get().uri("http://example.org")
            .retrieve()
            .bodyToMono(String.class);
      });
    })
    .transform(transformer)   // <=== so, "context" method may do this
    .subscribeOn(Schedulers.boundedElastic())
    .block();

Sorry, I wrote this in hurry, so didn't verify much, but this is something similar to what I do for MDC propagation. So, something like custom subscriber may work to propagate context.

@robotmrv
Copy link

@ttddyy
why to create custom subscriber if you can use oob operators?
something like this

webClient.get().uri("http://example.com")
    .retrieve()
    .bodyToMono(String.class)
    .publishOn(Schedulers.boundedElastic())
    .flatMap(str -> {
      return Mono.deferContextual(view -> {
        return webClient.get().uri("http://example.org")
            .retrieve()
            .bodyToMono(String.class);
      });
    })
    .transform(propagateToMono()) 
    .subscribeOn(Schedulers.boundedElastic())
    .block()
...

public static <T> Function<? super Mono<T>, ? extends Mono<T>> propagateToMono() {
    return it -> it.contextWrite(enrichCtx());
}

public static <T> Function<? super Flux<T>, ? extends Flux<T>> propagateToFlux() {
    return it -> it.contextWrite(enrichCtx());
}

public static Function<Context, Context> enrichCtx() {
    return ctx -> ctx.put("attr1", "value1");
}

@ttddyy
Copy link
Contributor Author

ttddyy commented Nov 12, 2020

Ah ok, I misunderstood.
So, context only works similar to attributes, on the other hand contextWrite propagates to nested ones.
It would be nice if context works similar to contextWrite if keeping the method.

The usage of contextWrite probably need to be more advertised especially in a context of ThreadLocal handling.

It is very common, in servlet environment, devs convert using RestTemplate to WebClient for various reasons, and pretty sure they will hit the issue about ThreadLocal (and MDC) soon.

In my case, I found nested webClient calls and a filter was referencing ThreadLocal value. Initially it was working because the filter used caller's thread. However, when code changed to introduce publishOn/subscribeOn, then it started failing because the executing thread now is different.

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Nov 13, 2020

@robotmrv as far as I can see Microprofile Context Propagation is about passing context across a declarative chain of CompletionStage instances. This is what the Reactor Context feature does for Flux and Mono so I don't see anything missing in terms of propagating context across a reactive chain.

For the transfer of data on a blocking callstack from ThreadLocal onto a reactive chain, it is straight forward to do so through Reactor operators added at the end of the reactive chain, possibly re-using a static function declared somewhere. It's what I intend to document for now as part of this issue.

To automate such such a transfer, there needs to be some a place in framework code that handles the result of a reactive chain on a blocking stack. Spring MVC controller methods are one such example. We could expose an extra config option, e.g. under AsyncSupportConfigurer, that accepts a function to populate the Reactor Context for Mono and Flux return values from controller methods which would allow to propagate ThreadLocal data to a reactive request handling chain.

To take the example from my previous comment, it would become slightly simpler:

@GetMapping(...)
public Mono<String> handle(...) {
	return client.get().uri("https://example.org/")
		.retrieve()
		.bodyToMono(String.class)
		.flatMap(body -> {
			// perform nested request...
		})
	// Drop the below and declare in global config (to be applied on the return value)
	//	.contextWrite(context -> ...);   
}

@robotmrv
Copy link

@rstoyanchev
Microprofile Context Propagation - creates snapshots of the desired contexts, and provides a way to propagate it back to the ThreadLocals in desired block of code in the unified way. Currently in Spring you have to do it manually or invent your custom tool.

Here is the case
I use Spring MVC and Reactive API on Controller level but still use some blocking library that depends on ThreadLocal contexts inside reactive chain. To not block potentially non-blocking thread I am switching execution to boundedElasticScheduler.
But the problem - ThreadLocal context is not filled within Function/Runnable/Supplier where blocking code is executed, so 3-rd party library does not work properly.

@GetMapping()
public Mono<String> getData() {
    return webClient.get()
            .uri("https://example.org/")
            .retrieve()
            .toBodilessEntity()
            .publishOn(Schedulers.boundedElastic())
            .map(data -> processDataByBlockingLibrary(data));//(1) uses ThreadLocal context, 
}

So there is a need of something that can transfer context data into reactive context and back to the ThreadLocal at (1) in uniform way, but not by custom tools.
I think this is common problem for everyone who tries to migrate gradually to reactive API or does not have reactive variant of 3-rd party dependency at the moment

@rstoyanchev
Copy link
Contributor

So there is a need of something that can transfer context data into reactive context

I made a concrete suggestion. Please, consider it.

and back to the ThreadLocal

This part doesn't make sense in a Spring MVC controller method. When you return a Mono, the Servlet container thread from which ThreadLocal information is sourced is released immediately, which the Mono completes later asynchronously.

@robotmrv
Copy link

robotmrv commented Nov 17, 2020

So there is a need of something that can transfer context data into reactive context

I made a concrete suggestion. Please, consider it.
I am ok with this part

This part doesn't make sense in a Spring MVC controller method. When you return a Mono, the Servlet container thread from which ThreadLocal information is sourced is released immediately, which the Mono completes later asynchronously.

It seems there is some misunderstanding about second part. I mean that processDataByBlockingLibrary(data) from example is invoked from reactive chain on different Thread but it requires some context data bounded to request which is usually saved in ThreadLocal.
As far as request is not over at processDataByBlockingLibrary(data) invocation point contextual data is still relevant. To be able work with contextual data, it should be propagated from reactor context to blocking context (i.e. into ThreadLocal).
Something similar to

//...
.flatMap(data -> Mono.deferContextual(contextView -> {
    fillThreadLocalContext(contextView);
    try {
        String result = processDataByBlockingLibrary(data);//uses ThreadLocal context
        return Mono.just(result);
    } finally {
        clearThreadLocalContext();
    }
}))
.contextWrite(captureContext());//as far as I understand this is going to be moved to some global config

fillThreadLocalContext() could propagate SecurityContext, LocaleContext, custom context etc that were captured in .contextWrite(captureContext())

update:

the Servlet container thread from which ThreadLocal information is sourced is released immediately

yes RequestAttributes released and you can't access attributes in async code, but RequestAttributes could be copied or captured only data that is needed like HttpServletRequest / HttpServletResponse from ServletRequestAttributes like spring-security does https://github.com/spring-projects/spring-security/blob/master/config/src/main/java/org/springframework/security/config/annotation/web/configuration/SecurityReactorContextConfiguration.java#L101

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Nov 17, 2020

Okay I see now, although this is a digression from the current issue which is about populating a Reactor Context from ThreadLocal storage. In a reactive chain we expect components to use the Reactor Context. This is very different from Microprofile and CompletableFuture where there is no other way to pass context.

As for blocking calls within a reactive chain, as a framework we don't know where those are and we can't incur the overhead of switching ThreadLocal values in and out at every stage just because there may or may not be a blocking call. It has to be a little more explicit and more targeted.

My earlier suggestion for what we can do in Spring MVC could be extended with some convention for a special Reactor Context variable that would be a Map of name-value pairs to use to populate ThreadLocals, e.g. via Schedulers.onSchedule. That would be a little more explicit but still not very targeted. An alternative would be to expose some primitives for blocking calls and context and let the application apply them where needed. Again, that would be a completely separate issue.

In the mean time an application can set up something like below:

private static <T> Mono<T> invokeBlocking(Callable<T> callable) {
	return Mono.deferContextual(view -> {
		Object someAttribute = view.get(SOME_ATTRIBUTE_NAME);
		// set ThreadLocal
		try {
			return Mono.just(callable.call());
		}
		catch (Exception ex) {
			return Mono.error(ex);
		}
		finally {
			// clear ThreadLocal
		}
	});
}

and apply it manually:

//...
.flatMap(data -> invokeBlocking(() -> processDataByBlockingLibrary(data))))
.contextWrite(captureContext());

This doesn't seem too different from Microprofile's Context Propagation which also enables this sort of wrapping of Callable, Runnable, and the like.

rstoyanchev added a commit that referenced this issue Nov 23, 2020
@rstoyanchev
Copy link
Contributor

The context method in WebClient is deprecated and will be removed in a subsequent maintenance release.

In the mean time the conversations here have given me ideas about improving the support in Spring MVC for transferring data from ThreadLocal's to Reactor Context and likewise to then re-establish ThreadLocal context within the reactive chain where imperative code is invoked that relies on it. That goes beyond the current issue and will be explored separately.

@ttddyy
Copy link
Contributor Author

ttddyy commented Nov 23, 2020

Thanks, looking forward to seeing how it will look like.

@jan-olaveide
Copy link

jan-olaveide commented Mar 31, 2022

Very late to the party here @rstoyanchev. What is the status for accessing RequestContext and other thread bound data in a WebClient used in a vanilla web-mvc/non-reactive controller?

@ngorantla-equinix
Copy link

Do we've any solution for it, i'm also having same problem. ServletRequest not able to inject in thread scope Webclient Request

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Sep 1, 2022

The solution is taking shape as a small, independent Context Propagation library. It happens to be under the Micrometer org because it's more generally useful, but does not depend on it. Support is being built into Reactor for version 3.5 and likewise for Spring Framework in version 6 but generally, there isn't much that we need to do in the Spring Framework, i.e. the library itself provides all the functionality and it can be used directly.

@rstoyanchev
Copy link
Contributor

There is now also #29056.

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) type: documentation A documentation task type: task A general task
Projects
None yet
Development

No branches or pull requests

6 participants