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

CookieServerCsrfTokenRepository#createNewToken should use Schedulers.boundedElastic #9018

Closed
tt4g opened this issue Sep 15, 2020 · 3 comments · Fixed by #9021
Closed

CookieServerCsrfTokenRepository#createNewToken should use Schedulers.boundedElastic #9018

tt4g opened this issue Sep 15, 2020 · 3 comments · Fixed by #9021
Assignees
Labels
in: web An issue in web modules (web, webmvc) status: backported An issue that has been backported to maintenance branches type: bug A general bug
Milestone

Comments

@tt4g
Copy link
Contributor

tt4g commented Sep 15, 2020

Describe the bug

Call UUID#randomUUID() in CookieServerCsrfTokenRepository#createNewToken() and UUID#randomUUID() is blocking I/O operation (reports #8128).
However, CookieServerCsrfTokenRepository#generateToken(ServerWebExchange) is not change Scheduler.

@Override
public Mono<CsrfToken> generateToken(ServerWebExchange exchange) {
return Mono.fromCallable(this::createCsrfToken);
}

private CsrfToken createCsrfToken() {
return createCsrfToken(createNewToken());
}

private String createNewToken() {
return UUID.randomUUID().toString();
}

To Reproduce

CookieServerCsrfTokenRepository#generateToken(ServerWebExchange)

Expected behavior

Use Schedulers.boundedElastic().

Sample

N/A

Is it the design of not using the Schedulers.boundedElastic()?

@tt4g tt4g added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Sep 15, 2020
@jzheaux
Copy link
Contributor

jzheaux commented Sep 16, 2020

Hi, @tt4g, thanks for the report.

I believe that it makes sense to change the token generation to use Schedulers.boundedElastic() like so:

Mono.fromCallable(this::createCsrfToken).subscribeOn(Schedulers.boundedElastic());

since UUID.randomUUID is blocking.

Would you be able to submit a PR to update the class?

@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 Sep 16, 2020
@jzheaux jzheaux self-assigned this Sep 16, 2020
@tt4g
Copy link
Contributor Author

tt4g commented Sep 16, 2020

I will submit PR in a few days.

@tt4g
Copy link
Contributor Author

tt4g commented Sep 16, 2020

@jzheaux PR has been sent. Could you please confirm?

tt4g added a commit to tt4g/spring-security that referenced this issue Sep 16, 2020
The CSRF token is generated by UUID.randomUUID() which is I/O blocking operation.
This commit changes the subscriber thread to the bounded elactic scheduler.

Closes spring-projectsgh-9018
jzheaux pushed a commit that referenced this issue Sep 16, 2020
The CSRF token is generated by UUID.randomUUID() which is I/O blocking operation.
This commit changes the subscriber thread to the bounded elactic scheduler.

Closes gh-9018
@jzheaux jzheaux added this to the 5.4.1 milestone Sep 16, 2020
@jzheaux jzheaux changed the title [Question} Why CookieServerCsrfTokenRepository does not use Schedulers.boundedElastic()? CookieServerCsrfTokenRepository#createNewToken should use Schedulers.boundedElastic Sep 16, 2020
jzheaux pushed a commit that referenced this issue Sep 16, 2020
The CSRF token is generated by UUID.randomUUID() which is I/O blocking operation.
This commit changes the subscriber thread to the bounded elactic scheduler.

Closes gh-9018
@spring-projects-issues spring-projects-issues added the status: backported An issue that has been backported to maintenance branches label Sep 16, 2020
jzheaux pushed a commit that referenced this issue Sep 16, 2020
The CSRF token is generated by UUID.randomUUID() which is I/O blocking operation.
This commit changes the subscriber thread to the bounded elactic scheduler.

Closes gh-9018
jzheaux pushed a commit that referenced this issue Sep 24, 2020
The CSRF token is generated by UUID.randomUUID() which is I/O blocking operation.
This commit changes the subscriber thread to the bounded elactic scheduler.

Closes gh-9018
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: backported An issue that has been backported to maintenance branches type: bug A general bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants