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

WebSessionServerRequestCache should avoid needless blocking operations #9200

Open
stokpop opened this issue Nov 12, 2020 · 6 comments
Open
Assignees
Labels
in: web An issue in web modules (web, webmvc) status: blocked An issue that's blocked on an external project change type: enhancement A general enhancement

Comments

@stokpop
Copy link

stokpop commented Nov 12, 2020

Using Spring Cloud Gateway and disabling the request cache we see different behaviour under load in our application: more latency and timeouts occur.

To disable we call: requestCache().disable()

Investigation showed that our custom filters in Spring Gateway run on a boundedElastic thread with request cache enabled and on the reactor-http-nio-* thread when request cache is disabled.

It seems that some blocking code in one of our filters is then causing the latency and timeouts on the reactor-http-nio-* threads and impact performance behaviour. (We are also looking into fixing this.)

Spring boot: 2.3.5.RELEASE
Spring cloud dependencies: Hoxton.SR8
spring-cloud-services-dependencies: 3.1.5.RELEASE

Not sure if spring-security is the correct place to post this issue, can also be a gateway or framework issue?

To Reproduce
With block hound enabled, we see no stacktrace for blocking code with request cache enabled (on bounded elastic thread), but we see the stacktrace for blocking code with request cache disabled (on reactor-http-nio thread).

Also, in logging you can see the thread that is used, which is different when cache is disabled.

Expected behaviour
Same threading behaviour expected when request cache is disabled/enabled.

No bounded elastic threads expected: spring cloud gateway threads should preferably not run on boundedElastic threads, as described here: spring-cloud/spring-cloud-gateway#1229

It might be related to this code org.springframework.web.server.session.InMemoryWebSessionStore#createWebSession from Spring framework:

@Override
	public Mono<WebSession> createWebSession() {

                 ...
		return Mono.<WebSession>fromSupplier(() -> new InMemoryWebSession(now))
				.subscribeOn(Schedulers.boundedElastic()); <== seem to cause boundedElastic Thread 
	}

With this subscribeOn call, it looks like more code is running on the bounded elastic thread than only the "new InMemoryWebSession(now)". The explicit subscribeOn was added for this issue: spring-projects/spring-framework#24027

From: https://projectreactor.io/docs/core/release/reference/#schedulers:
"Obtaining a Flux or a Mono does not necessarily mean that it runs in a dedicated Thread. Instead, most operators continue working in the Thread on which the previous operator executed. Unless specified, the topmost operator (the source) itself runs on the Thread in which the subscribe() call was made."

@stokpop stokpop added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Nov 12, 2020
@jzheaux jzheaux added in: web An issue in web modules (web, webmvc) and removed status: waiting-for-triage An issue we've not yet triaged labels Nov 24, 2020
@jzheaux jzheaux self-assigned this Nov 24, 2020
@jzheaux
Copy link
Contributor

jzheaux commented Nov 25, 2020

Thanks for the report, @stokpop.

I think you are correct that the issue is related to using WebSessionServerRequestCache. Have you already tried using CookieServerRequestCache? This cache remembers the last request without using the session.

@stokpop
Copy link
Author

stokpop commented Nov 30, 2020

Thanks for the suggestion, @jzheaux: I've set Spring Security to version 5.4.1 to use the new CookieServerRequestCache via ServerHttpSecurity:

        // use cookie server request cache: https://github.com/spring-projects/spring-security/issues/8033
        http.requestCache(cache -> cache.requestCache(new CookieServerRequestCache()));

I can confirm that the threads are running on the reactor-http-nio-* threads instead of boundedElastic threads with CookieServerRequestCache. (And I can see a cookie being set in the http response.) Still our goal is to disable the request cache completely and not have unexpected side effects due to default use of WebSessionServerRequestCache.

The default ServerHttpSecurity still seems to use RequestCacheSpec with default WebSessionServerRequestCache. So this will cause boundedElastic threads to be used instead of (preferred?) reactor-http-nio-* threads. Is that an issue?

@jzheaux jzheaux added the for: team-attention This ticket should be discussed as a team before proceeding label Nov 30, 2020
@jzheaux
Copy link
Contributor

jzheaux commented Nov 30, 2020

This is something that's been under consideration by the team in the past; @rwinch and @eleftherias probably have a bit more context. It might be best to have the default request cache be CookieServerRequestCache.

I'll bring up this ticket with the team and report back.

@DrongoX
Copy link

DrongoX commented Dec 21, 2020

hello @jzheaux !

did you have any news about the subj? I'm also curious about this change, but even in simpler context, such as just some REST API application based on netty+WebFlux. The fact that InMemoryWebSessionStore#createWebSession does .subscribeOn(Schedulers.boundedElastic()) leads to my reactive Controller code working on boudedElastic-* thread, which is bit confusing to me. Correct me if I'm wrong, but I would've expected some reactor-http-nio-* thread, as it was with previous versions.

Thanks!

@mddubey
Copy link

mddubey commented Apr 2, 2021

Hello Team,

Are there any updates on this? We are facing the same issue in spring webflux as a rest API and seems like this is leading to a lot of performance issues as well. Is there any update or workaround for this?

@jzheaux jzheaux changed the title Disabling RequestCache changes thread behaviour webflux/reactor. WebSessionServerRequestCache should avoid needless blocking operations May 19, 2021
@jzheaux jzheaux added type: enhancement A general enhancement and removed for: team-attention This ticket should be discussed as a team before proceeding type: bug A general bug labels May 19, 2021
@jzheaux
Copy link
Contributor

jzheaux commented May 19, 2021

I think that there's a potential improvement in WebFlux, so I've filed a ticket to take a look at adding a way to request a session without constructing one, similar to HttpServletRequest#getSession(false). I'll leave this ticket open depending on the progress of that ticket.

Having said that, I'll also mention that Reactor pipelines should not depend on the underlying thread. Being neutral to asynchronicity is the whole point of a Reactor pipeline. So, while it's an improvement to avoid needless blocking operations on each request, I don't see it as a bug that different Spring Security configurations result in similar pipelines being run on different sets of threads. For clarity, then, I've adjusted the ticket's title.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web An issue in web modules (web, webmvc) status: blocked An issue that's blocked on an external project change type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

4 participants