[main][WFLY-12329] managed executor service's execute(), submit() and schedule*() should use RequestController#forceBeginRequest() only when starting

[emmartins]

Issue: https://issues.redhat.com/browse/WFLY-12329
Please note that I was not able to add a test case to replicate the issue, due to being a race.

---

More information about the wildfly-bot[bot]

[bstansberry]

@emmartins My understanding is that in order to allow scheduled tasks to execute during startup this breaks server suspend / graceful shutdown by allowing tasks to be run after the server is suspended. If the schedule is recurring this will repeat.

Reading https://issues.redhat.com/browse/WFLY-12329?focusedId=13764490&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-13764490 it seems the reporter thinks the task should not be allowed to execute; they just don't like how WildFly handles not executing it.

I don't have great ideas about how to handle this in a way that makes sense for all the various ManagedScheduledExecutorService 'schedule' variants.

[emmartins]

@emmartins My understanding is that in order to allow scheduled tasks to execute during startup this breaks server suspend / graceful shutdown by allowing tasks to be run after the server is suspended. If the schedule is recurring this will repeat.

Yes, but only to a certain extent, while the tasks run there are the control points at the components. Honestly I was never a fan of plugging the controller in concurrency, since those are not components.

Reading https://issues.redhat.com/browse/WFLY-12329?focusedId=13764490&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-13764490 it seems the reporter thinks the task should not be allowed to execute; they just don't like how WildFly handles not executing it.

I don't have great ideas about how to handle this in a way that makes sense for all the various ManagedScheduledExecutorService 'schedule' variants.

There were a few other cases reported, which I resolved as dupes, with a different user perspective, but let me explain the way I see it...

There is a particular app use case needed by some users, which is the usage of a startup hook to do some work "once only", and there are 2 popular solutions for this: a singleton stateless EJB with a PostConstruct callback, or a CDI Observes+ApplicationScoped+Initialized event handler.

So I have checked EJB PostConstruct callback in the spec and there is nothing that would suggest Concurrency resources shouldn't work[1], as a matter of fact dependency injection and JNDI must be available and that points me to believe that such resources should then be available too. I didn't check but I believe same will be valid wrt the CDI solution.

Ok at this point I am assuming we should support this particular use case, the next question is how we ensure those concurrent resources are available to use on those hooks?

a) One solution would be to make the PostConstruct callback invocation to depend on all concurrent resource services/capabilities ("all" cause an app can lookup from JNDI with no hard dependency), same for CDI, and that would hit all apps with unnecessary dependencies, and also kind of defeat the purpose of EJB object pools warmups.

b) The other solution is we make the concurrent resources more passive wrt request controller, put them at same running "level" as those hooks, and that actually already is the case for ManagedExecutorService#execute() and ManagedExecutorService#submit(), which use the Controller's forceBeginRequest(). It's only ManagedScheduledExecutorService#schedule() methods that are not using forceBeginRequest(), and in fact that's why I gave users (last weeks in mail list) the workaround of replacing

scheduleAtFixedRate(runnable, delay=0, period, TimeUnit)

with

execute(runnable)
scheduleAtFixedRate(runnable, delay=period, period, TimeUnit)

...cause execute() uses forceBegin()

So in the end this PR just makes schedule() have the same behaviour as execute() and submit().

We can still discuss other solutions tho :-)

[1] https://jakarta.ee/specifications/enterprise-beans/4.0/jakarta-enterprise-beans-spec-core-4.0#a947

[bstansberry]

Ok at this point I am assuming we should support this particular use case, the next question is how we ensure those concurrent resources are available to use on those hooks?

+1. I think it makes sense to support it. Thanks for the FYI re other issues resolved as dups.

Other things besides EJBs that might be injected will not regulate invocations using a control point. The basic suspend concept focuses on cutting things off at the entry point.

Perhaps ManagedScheduledExecutorServiceImpl can depend on the org.wildfly.management.process-state-notifier capability and inject a Supplier. That can be used to provide an AtomicBoolean to doScheduledWrap calls, which can later be used to check if the process is starting and only use forceBeginRequest if so. I think the idea here is to allow these calls through when starting. If the schedule results in executing the runnable after that, it should use beginRequest and fail if the server is suspended.

There'd be a tiny race there between the isStarting check and calling on the controlPoint, but that seems ok. It might call 'forceBeginRequest' when it shouldn't, which is unlikely to be a problem.

[emmartins]

Ok at this point I am assuming we should support this particular use case, the next question is how we ensure those concurrent resources are available to use on those hooks?

+1. I think it makes sense to support it. Thanks for the FYI re other issues resolved as dups.

Other things besides EJBs that might be injected will not regulate invocations using a control point. The basic suspend concept focuses on cutting things off at the entry point.

Perhaps ManagedScheduledExecutorServiceImpl can depend on the org.wildfly.management.process-state-notifier capability and inject a Supplier. That can be used to provide an AtomicBoolean to doScheduledWrap calls, which can later be used to check if the process is starting and only use forceBeginRequest if so. I think the idea here is to allow these calls through when starting. If the schedule results in executing the runnable after that, it should use beginRequest and fail if the server is suspended.

There'd be a tiny race there between the isStarting check and calling on the controlPoint, but that seems ok. It might call 'forceBeginRequest' when it shouldn't, which is unlikely to be a problem.

Sounds good.

[emmartins]

@bstansberry done, please review...

Now execute(), submit(), schedule*() all use forceBeginRequest if process state is STARTING, beginRequest() otherwise.

Updated 32.x PR too, dunno if you still think this change is too much for a micro, if that's the case please let me know so I close that one and warn the community user that fix will only come with 33 Beta.

Thanks in advance.

[bstansberry]

/retest

[bstansberry]

Thanks @emmartins