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

Hot reload breaks when using suspending functions in controller with Quarkus security #35478

Open
FredyH opened this issue Aug 22, 2023 · 21 comments

Comments

@FredyH
Copy link

FredyH commented Aug 22, 2023

Describe the bug

When editing a suspending function in a Jakarta resource that is protected by Quarkus security (for example with the @PermitAll annotation), the reload will error and yield the following exception
Intercepted methods of the bean org.acme.ExampleResource may not be declared final:
This is likely due to the all-open plugin not being taken into account properly/too late.
It should be noted that the recompilation works fine if Quarkus security is not used.

Expected behavior

Live reload works correctly and new code is used

Actual behavior

Compilation error: Intercepted methods of the bean org.acme.ExampleResource may not be declared final:

How to Reproduce?

Reproducer: suspending-reproducer.zip

Steps:

  1. Start Quarkus in development mode
  2. Navigate to http://localhost:8080/hello
  3. Edit the text returned in ExampleResource::hello
  4. reload the page in the browser

Output of uname -a or ver

Microsoft Windows [Version 10.0.22621.2134]

Output of java -version

17.0.1

GraalVM version (if different from Java)

No response

Quarkus version or git rev

3.2.4.Final

Build tool (ie. output of mvnw --version or gradlew --version)

Gradle 8.1.1

Additional information

No response

@FredyH FredyH added the kind/bug Something isn't working label Aug 22, 2023
@quarkus-bot quarkus-bot bot added area/security env/windows Impacts Windows machines labels Aug 22, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented Aug 22, 2023

/cc @sberyozkin (security)

@sberyozkin
Copy link
Member

Hey @michalvavrik @mkouba Does it ring any bell ?

@mkouba
Copy link
Contributor

mkouba commented Aug 28, 2023

I'm no Kotlin expert but the error message is clear that "Kotlin suspend functions must be open and declared in open classes, otherwise they cannot be intercepted".

This is likely due to the all-open plugin not being taken into account properly/too late.

I don't think that the all-open plugin can be used in this particular case.

It should be noted that the recompilation works fine if Quarkus security is not used.

Yes, because Quarkus security is implemented with CDI interceptors. But any interceptor would cause the same problem though.

@mkouba mkouba added area/devmode and removed env/windows Impacts Windows machines labels Aug 28, 2023
@FredyH
Copy link
Author

FredyH commented Aug 28, 2023

I do not really understand, probably because I don't know how Quarkus hot-reload works under the hood.
The all-open plugin should tell the Kotlin compiler to make all relevant members open, including suspending functions. It works on the initial compile, so why would it not compile the functions to be open in the recompile?

It should be noted that other interceptors (not sure if it is CDI or not) appear to work correctly with all-open.
For example, if I remove the all-open plugin in the same reproducer above, then I will get the following compile error on start:

[error]: Build step io.quarkus.arc.deployment.ArcProcessor#validate threw an exception: jakarta.enterprise.inject.spi.DeploymentException: Intercepted methods of the bean org.acme.ExampleResource may not be declared final:
	- java.lang.Object hello(kotlin.coroutines.Continuation<? super java.lang.String> $completion)
	Kotlin `suspend` functions must be `open` and declared in `open` classes, otherwise they cannot be intercepted

However, the live reload works correctly with that interceptor (and not using security).

@mkouba
Copy link
Contributor

mkouba commented Aug 28, 2023

It works on the initial compile, so why would it not compile the functions to be open in the recompile?

I'm not sure about Kotlin but the regular Java classes are recompiled when modified in the dev mode.

@evanchooly do you have an idea if the all-open plugin is considered during recompilation in the dev mode?

It should be noted that other interceptors (not sure if it is CDI or not) appear to work correctly with all-open.

What interceptors do you have in mind?

@FredyH
Copy link
Author

FredyH commented Aug 28, 2023

I'm not sure about Kotlin but the regular Java classes are recompiled when modified in the dev mode.

In that case, if the all-open plugin is taken into account properly, it should also emit classes that are fully open.

What interceptors do you have in mind?

The one I posted above, I guess it is the ArcProcessor but I am not very familiar with how Quarkus works internally and whether or not this is the same kind of interceptor the security plugin uses.
Either way, it gives the same error if the all-open plugin is missing and the methods are not open, but it works correctly with the all-open plugin upon recompile.

@evanchooly
Copy link
Member

It works on the initial compile, so why would it not compile the functions to be open in the recompile?

I'm not sure about Kotlin but the regular Java classes are recompiled when modified in the dev mode.

@evanchooly do you have an idea if the all-open plugin is considered during recompilation in the dev mode?

It should be noted that other interceptors (not sure if it is CDI or not) appear to work correctly with all-open.

What interceptors do you have in mind?

I don't know how all that is set up. I thought it was designed to delegate to the build tool and simply reload the new class files.

@mkouba
Copy link
Contributor

mkouba commented Aug 29, 2023

What interceptors do you have in mind?

The one I posted above, I guess it is the ArcProcessor but I am not very familiar with how Quarkus works internally and whether or not this is the same kind of interceptor the security plugin uses. Either way, it gives the same error if the all-open plugin is missing and the methods are not open, but it works correctly with the all-open plugin upon recompile.

@FredyH ArcProcessor is no interceptor but a collection of build steps that are executed when the application is built. I was talking about CDI interceptors; Quarkus security is implemented with CDI interceptors, but any other feature implemented by a CDI interceptor (such as @Transactional) would cause the same issue.

I don't know how all that is set up. I thought it was designed to delegate to the build tool and simply reload the new class files.

@evanchooly There is the CompilationProvider abstraction and the KotlinCompilationProvider seems to be using Kotlin CLI and the plugins should be taken into account 🤷.

@mkouba mkouba added the area/gradle Gradle label Aug 29, 2023
@mkouba
Copy link
Contributor

mkouba commented Aug 29, 2023

@evanchooly There is the CompilationProvider abstraction and the KotlinCompilationProvider seems to be using Kotlin CLI and the plugins should be taken into account 🤷.

Except that CompilationProvider.Context.getCompilePluginArtifacts() returns an empty list and so does the CommonCompilerArguments.getPluginClasspaths()...

@mkouba
Copy link
Contributor

mkouba commented Aug 29, 2023

@aloubyansky @gsmet Do you happen to know if the plugins defined in the build.gradle(.kts) are "propagated" to the KotlinCompilationProvider used during the dev mode?

@FredyH
Copy link
Author

FredyH commented Aug 29, 2023

I did some more testing with @Transactional and it seems to have exactly the same problem as the security annotations, so it appears that you are correct and probably all CDI injectors are causing this error.

However, it should be noted that this only happens when the method is suspending. Changing a regular method that is not explicitly open, but rather only open because of the all-open plugin, reloads correctly. So the compiler plugin is being taken account at least in these cases.

It should be noted that suspend functions actually compile down to functions that are different than what is declared (taking a continuation and all), so maybe the issue somehow arises from that.

@michalvavrik
Copy link
Member

Do you happen to know if the plugins defined in the build.gradle(.kts) are "propagated" to the KotlinCompilationProvider used during the dev mode?

They are not, only DevMojo sets plugins in io.quarkus.maven.DevMojo#setKotlinSpecificFlags, so DevModeContext will never contains them when Gradle is used for it's only based on ApplicationModel. IMO the fact it works for Maven suggests it should be done.

@aloubyansky
Copy link
Member

Here is how compiler options are meant to be configured https://quarkus.io/guides/kotlin#configuring-live-reload-compiler

@sharubhat
Copy link

I have just started experimenting with Quarkus and Kotlin and this error bugged me. Adding custom annotation to classes and allOpen plugin didn't help. What worked for me is just add open keyword before suspend fun. Try it.

@michalvavrik
Copy link
Member

I have just started experimenting with Quarkus and Kotlin and this error bugged me. Adding custom annotation to classes and allOpen plugin didn't help. What worked for me is just add open keyword before suspend fun. Try it.

@sharubhat for what I remember when I stopped looking into it, this whole issue only happens when plugins are not configured as described in https://quarkus.io/guides/kotlin#configuring-live-reload-compiler. Please let me know if that impression is wrong, I could look again.

@sharubhat
Copy link

I have just started experimenting with Quarkus and Kotlin and this error bugged me. Adding custom annotation to classes and allOpen plugin didn't help. What worked for me is just add open keyword before suspend fun. Try it.

@sharubhat for what I remember when I stopped looking into it, this whole issue only happens when plugins are not configured as described in https://quarkus.io/guides/kotlin#configuring-live-reload-compiler. Please let me know if that impression is wrong, I could look again.

That did not help me. It just removed the stack trace from logs but the responses continued to error out.

@michalvavrik
Copy link
Member

I have just started experimenting with Quarkus and Kotlin and this error bugged me. Adding custom annotation to classes and allOpen plugin didn't help. What worked for me is just add open keyword before suspend fun. Try it.

@sharubhat for what I remember when I stopped looking into it, this whole issue only happens when plugins are not configured as described in https://quarkus.io/guides/kotlin#configuring-live-reload-compiler. Please let me know if that impression is wrong, I could look again.

That did not help me. It just removed the stack trace from logs but the responses continued to error out.

Noted, thanks.

@mschorsch
Copy link
Contributor

@michalvavrik Do you look into this? We are also very interested that this bug get fixed because we are migration to Gradle.

@mschorsch
Copy link
Contributor

similar issue #29875

@mschorsch
Copy link
Contributor

I've found a workaround: #37109 (comment)

@sharubhat
Copy link

I've found a workaround: #37109 (comment)

Thank you so much! Confirming that this works.

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

No branches or pull requests

8 participants