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

Destroy method not found in native image if concrete bean type is not exposed #29545

Closed
joshlong opened this issue Nov 22, 2022 · 4 comments
Closed
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) theme: aot An issue related to Ahead-of-time processing type: enhancement A general enhancement
Milestone

Comments

@joshlong
Copy link
Member

joshlong commented Nov 22, 2022

I have a spring boot 3 snapshots app with nothing on the classpath save spring-boot-starter with spring boot 3 aot and graalvm apps.

if I use the code:

package shutdown.demo;

import org.springframework.boot.SpringApplication;
import org.springframework.boot.autoconfigure.SpringBootApplication;
import org.springframework.context.annotation.Bean;

@SpringBootApplication
public class DemoApplication {

	public static void main(String[] args) {
		SpringApplication.run(DemoApplication.class, args);
	}

	@Bean
	Controller myController  (){
		return new ControllerManager();
	}
}

class ControllerManager implements Controller {

	@Override
	public void shutdown() {
		System.out.println("calling ControllerManager#shutdown" );
	}
}

interface Controller {

	void shutdown() ;
}

and then compile/run in a graalvm application, I get the following error:

022-11-21T17:56:24.631-08:00 INFO 66325 --- [ main] shutdown.demo.DemoApplication : No active profile set, falling back to 1 default profile: "default"
2022-11-21T17:56:24.633-08:00 WARN 66325 --- [ main] o.s.c.support.GenericApplicationContext : Exception encountered during context initialization - cancelling refresh attempt: org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'myController': Invalid destruction signature
2022-11-21T17:56:24.633-08:00 ERROR 66325 --- [ main] o.s.boot.SpringApplication : Application run failed

If I return ControllerManager from the @Bean method, that works.

It's still pretty dissatisfying when I have a builder that returns Controller, and not the subclass, and I have no way of knowing what subclass it is. Plus, it works on the JVM with no issues even with just the interface.

I know there is a best practice that we should use direct subclasses in @Bean method return types, but this feels like a different kind of bug, especially since shutdown is visible on the interface and the subclass.

The other issue here is that it is not my intent that this shutdown be used to clean up the bean when the application context shuts down. That is, I don’t want this to be used as the spring bean destroyMethod; it’s just a method that happened to be on the class. So to get these inscrutable errors about Invalid destruction signature is weird.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Nov 22, 2022
@snicoll snicoll transferred this issue from spring-projects/spring-boot Nov 22, 2022
@sbrannen sbrannen added this to the Triage Queue milestone Nov 22, 2022
@sbrannen sbrannen added in: core Issues in core modules (aop, beans, core, context, expression) theme: aot An issue related to Ahead-of-time processing labels Nov 22, 2022
@snicoll snicoll changed the title odd issue with destroy methods in an AOT context Destroy method not found if type is not exposed Nov 23, 2022
@snicoll
Copy link
Member

snicoll commented Nov 23, 2022

Thanks for the sample. I am afraid there's nothing we can do as the actual type is not exposed for AOT inference. This sample works on the JVM with AOT. It also works in Native if you add the following to the application:

hints.reflection().registerMethod(
		ReflectionUtils.findMethod(ControllerManager.class, "shutdown"),
		ExecutableMode.INVOKE);

It also works, as you found out yourself, if you expose the actual type (which is what you should be doing regardless of AOT).

It's still pretty dissatisfying when I have a builder that returns Controller, and not the subclass, and I have no way of knowing what subclass it is. Plus, it works on the JVM with no issues even with just the interface.

I can see how that can be problematic but it is the very nature of building a native image where things have to be known at build time. The shutdown method on ControllerManager simply doesn't exist because there's no way for us to know that type needs to be harvested.

The only thing I can see us doing is failing early if we detect a shutdown method on a type that isn't concrete. Flagging for team attention to see what the rest of the team thinks.

@snicoll
Copy link
Member

snicoll commented Nov 23, 2022

I forgot to mention that a proper fix for this would be to generate code for the method invocation, something like:

bd.onShutdown((instance) -> instance.shutdown());

@poutsma is considering exposing an API (#29553) that could help with this, I think.

@bclozel bclozel removed this from the Triage Queue milestone Jan 17, 2023
@sbrannen
Copy link
Member

The other issue here is that it is not my intent that this shutdown be used to clean up the bean when the application context shuts down. That is, I don’t want this to be used as the spring bean destroyMethod; it’s just a method that happened to be on the class. So to get these inscrutable errors about Invalid destruction signature is weird.

@joshlong, are you aware that the shutdown() method is being invoked due to default destroy method inference which looks for close() and shutdown() methods?

You can disable that inference by annotating your bean factory method with @Bean(destroyMethod = "").

If you run the following test class, you will not see ControllerManager#shutdown() invoked! printed to the console; however, if you switch back to @Bean you'll see the message printed to the console again.

@SpringJUnitConfig
class ShutdownTests {

	@Test
	void test() {
	}

	@Configuration
	static class Config {

		@Bean(destroyMethod = "")
		Controller myController() {
			return new ControllerManager();
		}
	}

	interface Controller {
		void shutdown();
	}

	static class ControllerManager implements Controller {

		@Override
		public void shutdown() {
			System.err.println("ControllerManager#shutdown() invoked!");
		}
	}

}

@sbrannen sbrannen changed the title Destroy method not found if type is not exposed Destroy method not found in native image if concrete bean type is not exposed Feb 21, 2023
@moray95
Copy link

moray95 commented Mar 9, 2023

Hello, I am facing a similar issue while trying to migrate an application to GraalVM native image and would like to give an alternate view as this issue appears to me to be much more troublesome then mentioned here.

I am trying to migrate an application using Spring Boot 3.0.4 and Hazelcast 5.2.1. When I boot up the application, I get a similar error for HazelcastInstance.shutdown method. When I look at the generated reflect-config.json, I can properly see a related entry:

{
  "name": "com.hazelcast.core.HazelcastInstance",
  "methods": [
    {
      "name": "shutdown",
      "parameterTypes": [ ]
    }
  ]
}

Possible workarounds:

  • Removing the destroyMethod: I am unable to easily do this as this bean is created by Spring Boot auto-configurations (and it probably wouldn't be a good idea anyway).
  • Exposing a bean with the concrete type: this is not practical as it's not really possible to determine the concrete type of the bean, again coming from Sprint Boot auto-configurations. Even the Spring Boot auto-configuration is unable to tell the concrete type as it's an implementation detail within the Hazelcast dependency.
  • Adding reflection configuration for the concrete types: Even though it's probably the best approach for now, it's very cumbersome since there are 7 different known concrete implementation of this interface. Moreover, new implementations might break the application. This method also takes away the abstraction provided by using an interface as it leaks implementation details.

Adding the fact there are probably many other auto-configurations that might cause similar issues, I think it's best if this can be solved within Spring.

Cause and possible solution:

After some trials with GraalVM native images, I have found that defining an interface in reflect-config does work when accessing the method trough the interface but not from the concrete type.

Example:

reflect-config.json:

[
  {
    "name": "org.example.Printer",
    "methods": [
      {
        "name": "print",
        "parameterTypes": [ ]
      }
    ]
  }
]
interface Printer {
    fun print()
}

class HelloWorldPrinter: Printer {
    override fun print() {
        println("Hello world")
    }
}

val printer: Printer = HelloWorldPrinter()

// This works
Printer::class.java.getMethod("print").invoke(printer)

// This doesn't work
HelloWorldPrinter::class.java.getMethod("print").invoke(printer)

It looks like Spring is using the second approach while searching for destroy methods. Transitioning to the first approach would most likely fix this issue. The bean definition type looks like is present in RootBeanDefinition as targetType, resolvedTargetType or factoryMethodReturnType (I might be wrong wrong on this as I am not familiar with Spring internals).

@sdeleuze sdeleuze self-assigned this Mar 10, 2023
@sdeleuze sdeleuze removed the status: waiting-for-triage An issue we've not yet triaged or decided on label Mar 10, 2023
@sdeleuze sdeleuze modified the milestones: 6.0.8, 6.0.7 Mar 10, 2023
sdeleuze added a commit to sdeleuze/spring-framework that referenced this issue Mar 30, 2023
After this commit, DisposableBeanAdapter can find destruction related methods
even when hints are just specified at interface level, which is typically the
case when a bean is exposed via one of its interfaces.

Closes spring-projectsgh-29545
@sdeleuze sdeleuze added the type: enhancement A general enhancement label Mar 30, 2023
sdeleuze added a commit to sdeleuze/spring-aot-smoke-tests that referenced this issue Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) theme: aot An issue related to Ahead-of-time processing type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

7 participants