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

IllegalStateException "The current thread cannot be blocked: vert.x-eventloop-thread-1" in kafka topic message processing while invoking SecurityIdentity.getPrincipal() #13835

Closed
albert0815 opened this issue Dec 11, 2020 · 15 comments · Fixed by #13866
Labels
area/security env/windows Impacts Windows machines kind/bug Something isn't working
Milestone

Comments

@albert0815
Copy link

Describe the bug
Using Quarkus 1.9.2 the very same code was working fine which is now failing with 1.10.3. I found a similar issue "OIDC 1.10.0.CR1 - event loop thread blocked #13249" but it was closed so I am not sure whether it is really the same problem.

We implemented a process which is receiving a message from a kafka topic and in that process we try to retrieve the name of the user if there is one with the following snippet (I mean, in this case there is no user, but in other cases where the code is used we have a user, and this class does more than just retrieving the name).

...
@Inject SecurityIdentity securityIdentity;
...
if (securityIdentity.getPrincipal() != null) {
  lastChangedBy = securityIdentity.getPrincipal().getName();
}
...

Quarkus started to throw an exception with 1.10 release:

2020-12-11 08:04:58,991 WARN  [io.sma.rea.mes.kafka] (vert.x-eventloop-thread-1) SRMSG18228: A failure has been reported for Kafka topics '[homologation-ui--feature-technical-value-ui-ci--ci-document-create]': java.lang.IllegalStateException: The current thread cannot be blocked: vert.x-eventloop-thread-1
        at io.smallrye.mutiny.operators.UniBlockingAwait.await(UniBlockingAwait.java:29)
        at io.smallrye.mutiny.groups.UniAwait.atMost(UniAwait.java:61)
        at io.smallrye.mutiny.groups.UniAwait.indefinitely(UniAwait.java:42)
        at io.quarkus.security.runtime.SecurityIdentityAssociation.getIdentity(SecurityIdentityAssociation.java:61)
        at io.quarkus.security.runtime.SecurityIdentityAssociation_ClientProxy.getIdentity(SecurityIdentityAssociation_ClientProxy.zig:250)
        at io.quarkus.security.runtime.SecurityIdentityProxy.getPrincipal(SecurityIdentityProxy.java:23)
        at io.quarkus.security.runtime.SecurityIdentityProxy_ClientProxy.getPrincipal(SecurityIdentityProxy_ClientProxy.zig:370)
        ...   
_SmallRyeMessagingInvoker_onMessage_f8d68f1a2268557ef7868a4206f95f280de86952.invoke(InformationDocumentCreationResource_SmallRyeMessagingInvoker_onMessage_f8d68f1a2268557ef7868a4206f95f280de86952.zig:48)

Expected behavior
getPrincipal() should return null in case no user can be retrieved.

Actual behavior
Exception is thrown

To Reproduce
In above mentioned issue you are describing that all await need to be removed. I checked this SecurityIdentityAssociation class, there are still some awaits. So I feel that maybe a reproducer might not be required as maybe the issue is clear. But if this is a different problem I can work on a reproducer, please let me know.

Environment (please complete the following information):
Output of uname -a or ver: Microsoft Windows [Version 10.0.17763.1282]
Output of java -version: openjdk version "11" 2018-09-25
GraalVM version (if different from Java):
Quarkus version or git rev: 1.10.3.Final
Build tool (ie. output of mvnw --version or gradlew --version): Apache Maven 3.6.3 (cecedd343002696d0abb50b32b541b8a6ba2883f)

@albert0815 albert0815 added the kind/bug Something isn't working label Dec 11, 2020
@ghost ghost added area/security env/windows Impacts Windows machines labels Dec 11, 2020
@ghost
Copy link

ghost commented Dec 11, 2020

/cc @sberyozkin

@geoand
Copy link
Contributor

geoand commented Dec 11, 2020

Are you using Reactive Messaging?

@albert0815
Copy link
Author

yes, this is part of a @incoming method.

@geoand
Copy link
Contributor

geoand commented Dec 11, 2020

Does the problem not go away if you add @Blocking?

@albert0815
Copy link
Author

Adding @Blocking solves the issue. If this is the intended solution this issue can be closed, sorry for bothering. Nevertheless it is not an obvious one I would say (given that it worked before). How would I know getPrincipal() is blocking?

@geoand
Copy link
Contributor

geoand commented Dec 11, 2020

Not a bother at all!

@sberyozkin should we document somewhere that getPrincipal() should not be called on the event-loop?

@gsmet
Copy link
Member

gsmet commented Dec 11, 2020

If we can't get the principal without blocking, I think we have an issue :)

/cc @cescoffier

@geoand
Copy link
Contributor

geoand commented Dec 11, 2020

If we can't get the principal without blocking, I think we have an issue :)

Why?

@sberyozkin
Copy link
Member

sberyozkin commented Dec 11, 2020

Hi Georgios @geoand, Guillaume @gsmet I agree here with Guillaume, securityIdentity.getPrincipal() should just work, this is a local property in QuarkusSecurityidentity instance

@sberyozkin
Copy link
Member

sberyozkin commented Dec 11, 2020

Though due to the proxification it is not going directly to it, not sure why it has to go through the internal thread local map; well - because it is a thread safe proxy :-)

@gsmet
Copy link
Member

gsmet commented Dec 11, 2020

@geoand my guess is that you will need the principal as soon as you're dealing with security. So if you can't get it without blocking, that means any secure endpoint will need to be blocking.

Now I have no idea how you deal with this when your users are stored in a database.

@sberyozkin
Copy link
Member

sberyozkin commented Dec 11, 2020

By the time the endpoint is invoked SecurityIdentity must've been populated, it feels like that it is a thread mismatch, no TL value is available, and as such an attempt to create a new SecurityIdentity is attempted - which is a problem resolved by adding a @Blocking annotation - presumably the same thread is used in this case ?

@cescoffier
Copy link
Member

@sberyozkin You should not use await anywhere. await blocks.
The principal should be retrieved and stored in the RoutingContext.

@stuartwdouglas
Copy link
Member

This is because of the changes around lazy authentication.

Basically lots of places that were using SecurityIdentity are now using Uni, as there is a chance that authentication has not actually happened yet, and subscribing to the Uni would trigger the authentication attempt. When proactive auth is in use (the default) await will never block.

I should be able to fix this, but the resulting code won't be quite as clean.

stuartwdouglas added a commit to stuartwdouglas/quarkus that referenced this issue Dec 14, 2020
This will no longer wrap the Identity in a Uni if
proactive auth is in use.

This also resolves a problem with setting the current
SecurityIdentity via a CDI event, which is not
compatible with lazy authentication.

Note that the problem can still occur if using lazy
auth from the IO thread, which is expected. To get
the identity in this situation you need to use
CurrentIdentityAssociation#getDeferredIdentity
and subscribe to the result.

Fixes quarkusio#13835
stuartwdouglas added a commit to stuartwdouglas/quarkus that referenced this issue Dec 14, 2020
This will no longer wrap the Identity in a Uni if
proactive auth is in use.

This also resolves a problem with setting the current
SecurityIdentity via a CDI event, which is not
compatible with lazy authentication.

Note that the problem can still occur if using lazy
auth from the IO thread, which is expected. To get
the identity in this situation you need to use
CurrentIdentityAssociation#getDeferredIdentity
and subscribe to the result.

Fixes quarkusio#13835
@sberyozkin
Copy link
Member

Hey Clement @cescoffier I promise I can repeat it if someone wakes me up at night and asks, what you should not do with Uni ? :-) I've learned my lesson after one of my infamous PRs got merged earlier :-)

stuartwdouglas added a commit to stuartwdouglas/quarkus that referenced this issue Dec 15, 2020
This will no longer wrap the Identity in a Uni if
proactive auth is in use.

This also resolves a problem with setting the current
SecurityIdentity via a CDI event, which is not
compatible with lazy authentication.

Note that the problem can still occur if using lazy
auth from the IO thread, which is expected. To get
the identity in this situation you need to use
CurrentIdentityAssociation#getDeferredIdentity
and subscribe to the result.

Fixes quarkusio#13835
stuartwdouglas added a commit to stuartwdouglas/quarkus that referenced this issue Dec 16, 2020
This will no longer wrap the Identity in a Uni if
proactive auth is in use.

This also resolves a problem with setting the current
SecurityIdentity via a CDI event, which is not
compatible with lazy authentication.

Note that the problem can still occur if using lazy
auth from the IO thread, which is expected. To get
the identity in this situation you need to use
CurrentIdentityAssociation#getDeferredIdentity
and subscribe to the result.

Fixes quarkusio#13835
stuartwdouglas added a commit to stuartwdouglas/quarkus that referenced this issue Dec 16, 2020
This will no longer wrap the Identity in a Uni if
proactive auth is in use.

This also resolves a problem with setting the current
SecurityIdentity via a CDI event, which is not
compatible with lazy authentication.

Note that the problem can still occur if using lazy
auth from the IO thread, which is expected. To get
the identity in this situation you need to use
CurrentIdentityAssociation#getDeferredIdentity
and subscribe to the result.

Fixes quarkusio#13835
@ghost ghost added this to the 1.11 - master milestone Dec 16, 2020
cemnura pushed a commit to cemnura/quarkus that referenced this issue Dec 22, 2020
This will no longer wrap the Identity in a Uni if
proactive auth is in use.

This also resolves a problem with setting the current
SecurityIdentity via a CDI event, which is not
compatible with lazy authentication.

Note that the problem can still occur if using lazy
auth from the IO thread, which is expected. To get
the identity in this situation you need to use
CurrentIdentityAssociation#getDeferredIdentity
and subscribe to the result.

Fixes quarkusio#13835
renegrob pushed a commit to renegrob/quarkus that referenced this issue Dec 23, 2020
This will no longer wrap the Identity in a Uni if
proactive auth is in use.

This also resolves a problem with setting the current
SecurityIdentity via a CDI event, which is not
compatible with lazy authentication.

Note that the problem can still occur if using lazy
auth from the IO thread, which is expected. To get
the identity in this situation you need to use
CurrentIdentityAssociation#getDeferredIdentity
and subscribe to the result.

Fixes quarkusio#13835
benkard added a commit to benkard/mulkcms2 that referenced this issue Sep 4, 2021
A bug causes authentication checks to fail when performed from inside
a reactive operation:

    quarkusio/quarkus#13835

To avoid such a reactive operation, we now render the HTML template
eagerly even in NewsletterResource#register, which is otherwise a
reactive implementation.

Change-Id: I26d6c1cc76eaa041a04c106b7cf06f024a0cded3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/security env/windows Impacts Windows machines kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants