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.

Sign up for GitHub

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

Jump to bottom

Add ThreadLocalAccessor for LocaleContext and RequestAttributes #32243

Closed
wants to merge 1 commit into from
Closed

Add ThreadLocalAccessor for LocaleContext and RequestAttributes #32243

wants to merge 1 commit into from

Conversation

ttddyy
Copy link
Contributor

@ttddyy ttddyy commented Feb 12, 2024

Add ThreadLocalAccessor implementations:

  • LocaleThreadLocalAccessor
  • RequestAttributesThreadLocalAccessor

Closes gh-32112

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Feb 12, 2024
@sbrannen sbrannen added in: web Issues in web modules (web, webmvc, webflux, websocket) in: core Issues in core modules (aop, beans, core, context, expression) labels Feb 12, 2024
@sbrannen sbrannen changed the title Add ThreadLocalAccessor for LocaleContext and RequestAttributes Add ThreadLocalAccessor for LocaleContext and RequestAttributes Feb 12, 2024
@snicoll snicoll self-assigned this Feb 12, 2024
@snicoll snicoll changed the title Add ThreadLocalAccessor for LocaleContext and RequestAttributes Add ThreadLocalAccessor for LocaleContext and RequestAttributes Feb 12, 2024
@snicoll snicoll added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Feb 12, 2024
@snicoll snicoll added this to the 6.2.0-M1 milestone Feb 12, 2024
@snicoll snicoll mentioned this pull request Feb 12, 2024
Closed
rstoyanchev
rstoyanchev reviewed Feb 12, 2024
View reviewed changes
Copy link
Contributor

@rstoyanchev rstoyanchev left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. Leaving some feedback to consider.

framework-docs/modules/ROOT/pages/web/webmvc/mvc-ann-async.adoc Outdated
the option to opt-in to context propagation through the `ThreadLocalAccessor` implementations,
`LocaleContextThreadLocalAccessor` and `RequestAttributesThreadLocalAccessor`.
(They are not auto-registered by default.)

Copy link
Contributor

@rstoyanchev rstoyanchev Feb 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these implementations can be mentioned at the end of this section, after the scenarios are discussed. Something along the lines of..

The following ThreadLocalAccessor implementations are provided out of the box:

  • LocaleContextThreadLocalAccessor -- propagates LocaleContext via `LocalContetHolder.
  • ...

The above are not registered automatically. You will need to register them via ContextRegistry.getInstance() on startup.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this change. However, I still think this whole addition needs to move to end of the section. Otherwise it breaks up the flow, and the next paragraph "For other asynchronous handling scenarios..." is now referring to content that's further up.

snicoll
snicoll reviewed Feb 12, 2024
View reviewed changes
...context/src/main/java/org/springframework/context/i18n/LocaleContextThreadLocalAccessor.java Outdated
@@ -0,0 +1,55 @@
/*
* Copyright 2024-2024 the original author or authors.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The copyright header must be 2002-2024. This applies to other files as well.

...xt/src/test/java/org/springframework/context/i18n/LocaleContextThreadLocalAccessorTests.java
* @author Tadaya Tsuyukubo
*/
class LocaleContextThreadLocalAccessorTests {
private final ContextRegistry registry = new ContextRegistry()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New line required here

...context/src/main/java/org/springframework/context/i18n/LocaleContextThreadLocalAccessor.java Outdated
* A {@link ThreadLocalAccessor} to put and restore the current {@link LocaleContext}.
*
* @author Tadaya Tsuyukubo
* @since 6.2.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use 6.2 for things introduced in the first release of a line.

.../java/org/springframework/web/context/request/RequestAttributesThreadLocalAccessorTests.java
import static org.mockito.Mockito.mock;

/**
* @author Tadaya Tsuyukubo
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add Tests for {@link RequestAttributesThreadLocalAccessor}.

.../java/org/springframework/web/context/request/RequestAttributesThreadLocalAccessorTests.java Outdated
latch.countDown();
}).start();

latch.await(10, TimeUnit.MILLISECONDS);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems rather short to me.

@ttddyy ttddyy force-pushed the gh-32112_thread-local-accessor branch from e6967c2 to 6ab8517 Compare February 13, 2024 06:21
@ttddyy
Copy link
Contributor Author

ttddyy commented Feb 13, 2024

Updated with the followings:

  • Copyright year
  • @since to 6.2
  • TLA key names as <FQDN> + .KEY
  • Increase latch max wait time to 1sec in tests
  • Documentation

@ttddyy
Add ThreadLocalAccessor for LocaleContext and RequestAttributes
991b161 
Add `ThreadLocalAccessor` implementations:
- `LocaleThreadLocalAccessor`
- `RequestAttributesThreadLocalAccessor`

Closes spring-projectsgh-32112

Signed-off-by: Tadaya Tsuyukubo <[email protected]>
@ttddyy ttddyy force-pushed the gh-32112_thread-local-accessor branch from 6ab8517 to 991b161 Compare February 13, 2024 22:25
snicoll pushed a commit that referenced this pull request Feb 15, 2024
@ttddyy @snicoll
Add ThreadLocalAccessor for LocaleContext and RequestAttributes
5bd1c1f 
Add `ThreadLocalAccessor` implementations:
- `LocaleThreadLocalAccessor`
- `RequestAttributesThreadLocalAccessor`

See gh-32243
snicoll added a commit that referenced this pull request Feb 15, 2024
@snicoll
Polish "Add ThreadLocalAccessor for LocaleContext and RequestAttributes"
aef4b21 
See gh-32243
@snicoll snicoll closed this in 4b2e401 Feb 15, 2024
@rstoyanchev rstoyanchev mentioned this pull request Feb 19, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rstoyanchev rstoyanchev rstoyanchev left review comments

@snicoll snicoll snicoll left review comments

Assignees

@snicoll snicoll

Labels
in: core Issues in core modules (aop, beans, core, context, expression) in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Milestone
6.2.0-M1
Development

Successfully merging this pull request may close these issues.

ThreadLocalAccessor implementation for RequestAttributes and LocaleContext
5 participants
@ttddyy @rstoyanchev @snicoll @sbrannen @spring-projects-issues