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

Update to Micronaut 2.0.0 breaks injection of DefaultSecurityService in background tasks #3868

Open
oscarfh opened this issue Aug 5, 2020 · 11 comments

Comments

@oscarfh
Copy link

oscarfh commented Aug 5, 2020

Hi,
I am migrating a project from Micronaut 1.x to 2.0 and I am facing a behavior that was changed and I am wondering whether this is something I can configure and that I should change entirely.

My code is something like this (simplified version)

public class MyClass() {
	@Inject
	private DefaultSecurityService defaultSecurityService;

	public CompletableFuture<Boolean> check() {
		Optional<Authentication> accessToken = defaultSecurityService.getAuthentication(); //Here is works
		return CompletableFuture.supplyAsync(() -> {
			Optional<Authentication> accessToken2 = defaultSecurityService.getAuthentication(); //Here is is Optional.empty
			// do some stuff, including calling a HttpClient
		}
	}
}

The problem is that I have a filter down the chain that uses defaultSecurityService.getAuthentication();. In 1.x, I was able to get the user's authentication and create a token for it. Now I can't and my request is made without token and fails.

My filter

@Filter("/path/**")
public class AuthHeaderFilter implements HttpClientFilter {
	@Inject
	private DefaultSecurityService defaultSecurityService;

	@Override
	public Publisher<? extends HttpResponse<?>> doFilter(MutableHttpRequest<?> request, ClientFilterChain chain) {
		return chain.proceed(request.bearerAuth(getToken(defaultSecurityService)); //I cannot generate the token here anymore
	}

Question:
Can I propagate the user context to the new thread, so my filter works as before?
(I need the security service in my filter, I added it to the check method in order to verify whether the behavior was due to the async thread or some change on the filter).

Clarifications:

  • I am using Async because I make multiple http call async and then combine the result of the calls

Steps to Reproduce:
Checkout this commit in this repository and Run the com.example.DemoTest#testHello test. You will be able to see to following output in your logs:

defaultSecurityService.getAuthentication(): Optional[io.micronaut.security.authentication.AuthenticationUserDetailsAdapter@772bace8]
defaultSecurityService.getAuthentication(): Optional[io.micronaut.security.authentication.AuthenticationUserDetailsAdapter@772bace8]

Which indicates that the context was inherited by the backgroup thread.

Then check out this commit, clean and **rebuild ** and you will see the following output:

defaultSecurityService.getAuthentication(): Optional[io.micronaut.security.authentication.AuthenticationUserDetailsAdapter@2b471c18]
defaultSecurityService.getAuthentication(): Optional.empty

Indicating that the context is not propagated anymore.

@someshrathi02
Copy link

What is error you are getting..? if its circular dependency related then pls see below .. #3239
You may need to use Provider

@oscarfh
Copy link
Author

oscarfh commented Aug 10, 2020

@someshrathi02 No error, but defaultSecurityService.getAuthentication() returns Optional.empty instead of the authentication object.

@oscarfh
Copy link
Author

oscarfh commented Aug 10, 2020

@someshrathi02 I edited the original post with a project to reproduce the issue.

@someshrathi02
Copy link

Thanks foe updates. so its different issue than one which i faced while upgrading micronaut version

@dstepanov
Copy link
Contributor

@oscarfh Can you use RxJava2's instead of CompletableFuture::supplyAsync?

@oscarfh
Copy link
Author

oscarfh commented Aug 17, 2020

@dstepanov yes, I did and it worked. Nevertheless, in my opinion, the fact that the context was passed to the new thread and now it is not anymore is an important behavior change that should, at least, be documented.

@graemerocher
Copy link
Contributor

we probably need a way to handle this use case although I believe CompletableFuture::supplyAsync uses the fork join common pool which is not ideal for the use case described and is supposed to be used for short lived, non-blocking operations.

We may be able to support the use case whereby supplyAsync is used with a specific ExecutorService that you can wrap in instrumentation. Thoughts @dstepanov ?

@dstepanov
Copy link
Contributor

Actually changing the example to

    @Inject
    @Named(TaskExecutors.IO)
    private ExecutorService executorService;

It correctly injects an instrumented Executor and context is preserved.

@graemerocher Looking at this commit 928a2ba should it be if (beanType == ExecutorService.class || beanType == ScheduledExecutorService.class)?

@dstepanov
Copy link
Contributor

@oscarfh I think this can be closed, the problem you have that without a name you are injecting "random" ExecutorService, I have tried to check in the debugger and I saw Netty's service which shouldn't be used.

@graemerocher
Copy link
Contributor

@dstepanov interesting that Micronaut is injecting a random, it should fail with a multiple bean definition exception without @Named. That is probably a bug in itself.

@oscarfh
Copy link
Author

oscarfh commented Aug 26, 2020

@dstepanov @graemerocher If you feel it is more appropriate, feel free to close.
Thanks a lot for the thread!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants