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

LocaleContextHolder should have a strategy similar to SecurityContextHolder for child Thread [SPR-17604] #22136

Closed
spring-issuemaster opened this issue Dec 17, 2018 · 2 comments

Comments

@spring-issuemaster
Copy link
Collaborator

@spring-issuemaster spring-issuemaster commented Dec 17, 2018

Timothy Freyne opened SPR-17604 and commented

The current LocaleContextHolder uses ThreadLocal to store the locale which works fine if you keep your execution to that thread. However in a multithreaded environment, the ThreadLocal value is not available when a new thread is started from that original thread.

Consider the following flow:

  1. in Spring MVC a GET request is being processed with 'Accept-Language' = 'nl'
  2. Controller that handles the GET request can read the value using LocaleContextHolder.getLocale().getLanguage()
  3. Controller processes some actions in parallel using multiple threads Thread A, Thread B
  4. When calling LocaleContextHolder.getLocale().getLanguage() from Thread A and B, the value 'nl' is not available.

In Spring Security this is already taken care of using org.springframework.security.core.context.InheritableThreadLocalSecurityContextHolderStrategy. The LocaleContextHolder should be coded in a similar way in order to switch threads and have the original value in the child thread.


Affects: 5.1.3

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Dec 17, 2018

Juergen Hoeller commented

There is support for this already through the inheritable argument on the LocaleContextHolder setters. This can also be declaratively enabled through DispatcherServlet's threadContextInheritable configuration property.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Dec 17, 2018

Timothy Freyne commented

Thank you for your response. The documentation mentions to not use when using a thread pool.

/**
 * Set whether to expose the LocaleContext and RequestAttributes as inheritable
 * for child threads (using an {@link java.lang.InheritableThreadLocal}).
 * <p>Default is "false", to avoid side effects on spawned background threads.
 * Switch this to "true" to enable inheritance for custom child threads which
 * are spawned during request processing and only used for this request
 * (that is, ending after their initial task, without reuse of the thread).
 * <p><b>WARNING:</b> Do not use inheritance for child threads if you are
 * accessing a thread pool which is configured to potentially add new threads
 * on demand (e.g. a JDK {@link java.util.concurrent.ThreadPoolExecutor}),
 * since this will expose the inherited context to such a pooled thread.
 */
public void setThreadContextInheritable(boolean threadContextInheritable) {
   this.threadContextInheritable = threadContextInheritable;
}

We are using a thread pool and Spring Security makes this work by using org.springframework.security.task.DelegatingSecurityContextAsyncTaskExecutor like this:

@Configuration
@EnableAsync
public class AsyncConfig implements AsyncConfigurer {
    ...
    @Override
    public Executor getAsyncExecutor() {
        ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor();
        executor.setCorePoolSize(corePoolSize);
        ...
        return new DelegatingSecurityContextAsyncTaskExecutor(executor);
    }

Would be great if this was all combined into a single "it-just-works" feature in all cases. It seems that for LocaleContextHolder, even though DispatcherServlet.setThreadContextInheritable(true) is available, does not work in all cases like Spring Security and we MUST pass the locale to these calls. Or am I missing something here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.