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

@Scheduled method invoked both on original bean and @Alternative bean #24212

Closed
aluckenbaugh opened this issue Mar 9, 2022 · 9 comments · Fixed by #24270
Closed

@Scheduled method invoked both on original bean and @Alternative bean #24212

aluckenbaugh opened this issue Mar 9, 2022 · 9 comments · Fixed by #24270
Assignees
Labels
area/scheduler kind/bug Something isn't working
Milestone

Comments

@aluckenbaugh
Copy link

Describe the bug

If @Scheduled is applied to a function and an @Alternative subclass is created for the bean containing the annotated method, the method is invoked twice by the scheduler, once for each bean. This means that when the alternative bean is being used the scheduled method is invoked twice, which is unexpected.

A reproducer is attached with the minimal changes necessary to show this, applied on the scheduler-quickstart project.

Our use case is we are creating a bean like CounterBean in the attached example that lives inside a library. This CounterBean could be subclassed and customized by providing an alternative like AlternativeCounterBean in different projects that use the library, and if that is done at the moment we find multiple invocations of the scheduled method, one immediately after the other.

Expected behavior

Only one invocation of the @Scheduled method should be performed on the alternative bean; the original bean's invocation should not be performed. In the given example, only AlternativeCounterBean#increment() should be invoked by the scheduler.

Actual behavior

As can be seen in the log output, both the CounterBean#increment() and the AlternativeCounterBean#increment() are invoked by the scheduler.

[INFO] Running org.acme.scheduler.CountResourceTest
2022-03-09 12:40:11,111 INFO [org.jbo.threads] (main) JBoss Threads version 3.4.2.Final
2022-03-09 12:40:12,757 INFO [io.quarkus] (main) Quarkus 2.7.3.Final on JVM started in 2.108s. Listening on: http://localhost:8081
2022-03-09 12:40:12,758 INFO [io.quarkus] (main) Profile test activated.
2022-03-09 12:40:12,758 INFO [io.quarkus] (main) Installed features: [cdi, resteasy, scheduler, smallrye-context-propagation, vertx]
2022-03-09 12:40:13,011 INFO [org.acm.sch.CounterBean] (executor-thread-1) Invoked from: org.acme.scheduler.CounterBean
2022-03-09 12:40:13,011 INFO [org.acm.sch.CounterBean] (executor-thread-0) Invoked from: org.acme.scheduler.AlternativeCounterBean
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 6.313 s - in org.acme.scheduler.CountResourceTest
2022-03-09 12:40:13,968 INFO [io.quarkus] (main) Quarkus stopped in 0.048s

How to Reproduce?

scheduler-quickstart-reproducer.zip

Output of uname -a or ver

Microsoft Windows [Version 10.0.19042.1526]

Output of java -version

openjdk version "11.0.9.1" 2020-11-04 LTS OpenJDK Runtime Environment Corretto-11.0.9.12.1 (build 11.0.9.1+12-LTS) OpenJDK 64-Bit Server VM Corretto-11.0.9.12.1 (build 11.0.9.1+12-LTS, mixed mode)

GraalVM version (if different from Java)

No response

Quarkus version or git rev

2.7.3.Final

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

Apache Maven 3.8.4 (9b656c72d54e5bacbed989b64718c159fe39b537) Maven home: C:\ProgramData\Chocolatey\lib\maven\apache-maven-3.8.4 Java version: 11.0.9.1, vendor: Amazon.com Inc., runtime: C:\Program Files\Amazon Corretto\jdk11.0.9_12 Default locale: en_US, platform encoding: Cp1252 OS name: "windows 10", version: "10.0", arch: "amd64", family: "windows"

Additional information

No response

@aluckenbaugh aluckenbaugh added the kind/bug Something isn't working label Mar 9, 2022
@quarkus-bot quarkus-bot bot added area/scheduler env/windows Impacts Windows machines labels Mar 9, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented Mar 9, 2022

/cc @mkouba

@mkouba
Copy link
Contributor

mkouba commented Mar 10, 2022

Note that @Alternative does not disable the original bean. It merely gives higher priority to @Alternative beans during injection. For example, if you have @Alternative class MockFoo extends Foo and Foo declares an observer method then this observer method is also invoked upon the Foo bean instance. In other words, I think that @Alternative itself should not disable the @Scheduled methods declared on superbeans.

However, in your particular case I'd say that only the scheduled method declared on CounterBean should be executed because the overriding method declared on AlternativeCounterBean is not annotated with @Scheduled.

@machi1990 @manovotn I think that we should clarify the inheritance rules for @Scheduled methods. WDYT?

@aluckenbaugh In your case I'd recommend to separate the logic that should be executed (and can be overriden) and the scheduled method itself (here the @Alternative would work as expected).

@ApplicationScoped
public class ScheduledBean {

   @Inject
   Logic logic;

   @Scheduled(every = "10s")
   void execute() {
       logic.execute();
   }
}

@ApplicationScoped
class Logic {
  void execute() {
    // do something
  }
}

@Alternative
@ApplicationScoped
class AlternativeLogic extends Logic {
  void execute() {
    // do something
  }
}

You can also consider using the default beans:

@DefaultBean
@ApplicationScoped
class Logic {
  void execute() {
    // do something
  }
}

@ApplicationScoped
class AlternativeLogic extends Logic {
  void execute() {
    // do something
  }
}

@aluckenbaugh
Copy link
Author

Thank you for your response, @mkouba. Your recommendation for separating the logic and the scheduling in different beans provides us with a usable pattern.

However, in your particular case I'd say that only the scheduled method declared on CounterBean should be executed because the overriding method declared on AlternativeCounterBean is not annotated with @Scheduled.

Would this mean that the behavior of the Quarkus scheduler needs to change? At the moment in the code example I provided both the AlternativeCounterBean and CounterBean instances are having their increment() functions invoked by the scheduler.

This behavior seems to cause a problem if an identity is specified in the scheduled annotation on the CounterBean, like this:

@Scheduled(every = "5s", identity = "counter_bean_identity")
void increment() {
    logger.info("Invoked from: {}", getClass().getCanonicalName());
    counter.incrementAndGet();
}

In that case with the AlternativeCounterBean defined like this, Quarkus fails to start with the following error:

@Alternative
@Priority(1)
@ApplicationScoped
public class AlternativeCounterBean extends CounterBean {
   
   @Override
   void increment() {
      super.increment();
   }
}

Error:

2022-03-10 08:36:06,934 ERROR [io.qua.dep.dev.IsolatedDevModeMain] (main) Failed to start quarkus: java.lang.RuntimeException: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
[error]: Build step io.quarkus.arc.deployment.ArcProcessor#generateResources threw an exception: javax.enterprise.inject.spi.DeploymentException: java.lang.IllegalStateException: The identity: "counter_bean_identity" on: @scheduled(every = "5s",identity = "counter_bean_identity") is not unique and it has already bean used by : @scheduled(every = "5s",identity = "counter_bean_identity")
at io.quarkus.arc.processor.BeanDeployment.processErrors(BeanDeployment.java:1202)
at io.quarkus.arc.processor.BeanProcessor.processValidationErrors(BeanProcessor.java:147)
at io.quarkus.arc.deployment.ArcProcessor.generateResources(ArcProcessor.java:492)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:566)
at io.quarkus.deployment.ExtensionLoader$2.execute(ExtensionLoader.java:882)
at io.quarkus.builder.BuildContext.run(BuildContext.java:277)
at org.jboss.threads.ContextHandler$1.runWith(ContextHandler.java:18)
at org.jboss.threads.EnhancedQueueExecutor$Task.run(EnhancedQueueExecutor.java:2449)
at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1478)
at java.base/java.lang.Thread.run(Thread.java:829)
at org.jboss.threads.JBossThread.run(JBossThread.java:501)
Caused by: java.lang.IllegalStateException: The identity: "counter_bean_identity" on: @scheduled(every = "5s",identity = "counter_bean_identity") is not unique and it has already bean used by : @scheduled(every = "5s",identity = "counter_bean_identity")
at io.quarkus.scheduler.deployment.SchedulerProcessor.validateScheduled(SchedulerProcessor.java:451)
at io.quarkus.scheduler.deployment.SchedulerProcessor.validateScheduledBusinessMethods(SchedulerProcessor.java:200)
... 11 more

If I don't use the explicitly defined identity and annotate the overriding method on AlternativeCounterBean with @Scheduled as well, there are three invocations, and if I understood your comment correctly it seems that there should be only two in that case:

2022-03-10 08:46:56,022 INFO [org.acm.sch.CounterBean] (executor-thread-0) Invoked from: org.acme.scheduler.AlternativeCounterBean_Subclass
2022-03-10 08:46:56,021 INFO [org.acm.sch.CounterBean] (executor-thread-2) Invoked from: org.acme.scheduler.CounterBean_Subclass
2022-03-10 08:46:56,022 INFO [org.acm.sch.CounterBean] (executor-thread-1) Invoked from: org.acme.scheduler.AlternativeCounterBean_Subclass

Is this its intended behavior?

@mkouba
Copy link
Contributor

mkouba commented Mar 10, 2022

Would this mean that the behavior of the Quarkus scheduler needs to change?

Yes, and given the fact that scheduled methods with identity may cause troubles we'll need to think about this and find some acceptable solution. TBH we did not think a lot about @Scheduled and inheritance because it's not a common use case.

@mkouba
Copy link
Contributor

mkouba commented Mar 10, 2022

FTR the "inheritance" for @Scheduled methods was introduced in this commit: 54dfc3b (when Scheduled#identity() did not exist yet)

@mkouba
Copy link
Contributor

mkouba commented Mar 10, 2022

Hm, I think that @Scheduled methods should not be inherited at all. The identity must be unique and it would be very confusing if scheduled methods with identity would not be inherited and methods without identity woud be inherited. @machi1990 WDYT?

It would be a breaking change but I'm +1 since the current behavior does not make a lot of sense. AFAIK it's an undocumented feature anyway.

@machi1990
Copy link
Member

@machi1990 @manovotn I think that we should clarify the inheritance rules for @scheduled methods. WDYT?
Hm, I think that @scheduled methods should not be inherited at all. The identity must be unique and it would be very confusing if scheduled methods with identity would not be inherited and methods without identity woud be inherited. @machi1990 WDYT?
It would be a breaking change but I'm +1 since the current behavior does not make a lot of sense. AFAIK it's an undocumented feature anyway.

+1 on having consistent behavior that can be clearly documented. So I am in favor of making the @Scheduled method not inherited. It will be good if we could still support declaring @Scheduled method on interface like below. We could validate during build time if there is more than one implementation of the interface

public interface ICounterBean {
   @Scheduled(identity="the idenitity")
   void increment() 
}

public class CounterBean implements ICounterBean {
   @Override
   void increment() {
     // actual implementation
   }
}

@machi1990 machi1990 removed the env/windows Impacts Windows machines label Mar 10, 2022
@mkouba
Copy link
Contributor

mkouba commented Mar 10, 2022

It will be good if we could still support declaring @scheduled method on interface like below

Hm, this is not supported ATM and to be honest, I don't see any benefit in this pattern...

@machi1990
Copy link
Member

It will be good if we could still support declaring @scheduled method on interface like below

Hm, this is not supported ATM and to be honest, I don't see any benefit in this pattern...

Thanks, I wrote the snippet with a vague recollection of it being supported but I was wrong.

@mkouba mkouba self-assigned this Mar 11, 2022
@mkouba mkouba added this to the 2.8 - main milestone Mar 11, 2022
mkouba added a commit to mkouba/quarkus that referenced this issue Mar 11, 2022
- this is a breaking change
- however, the current behavior is undocumented and inconsistent; e.g.
if Scheduled#identity() is used then the build always fails
- also the scope annotation is not added automatically to a subclass of a class that
declares a Scheduled annotation, i.e. the class would be ignored and would
result in runtime error
- resolves quarkusio#24212
mkouba added a commit to mkouba/quarkus that referenced this issue Mar 11, 2022
- this is a breaking change
- however, the current behavior is undocumented and inconsistent; e.g.
if Scheduled#identity() is used then the build always fails
- also the scope annotation is not added automatically to a subclass of a class that
declares a Scheduled annotation, i.e. the class would be ignored and would
result in runtime error
- resolves quarkusio#24212
mkouba added a commit to mkouba/quarkus that referenced this issue Mar 11, 2022
- this is a breaking change
- however, the current behavior is undocumented and inconsistent; e.g.
if Scheduled#identity() is used then the build always fails
- also the scope annotation is not added automatically to a subclass of a class that
declares a Scheduled annotation, i.e. the class would be ignored and would
result in runtime error
- resolves quarkusio#24212
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/scheduler kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants