Possible shutdown enhancements

[jgslima]

I can think of 2 possible enhancements for the shutdown/stop procedure

Stop the MessageSink before

AbstractPipelineMessageListenerContainer.doStop() does this:

LifecycleHandler.get().stop(this.messageSources, this.messageSink);

LifecycleHandler.stop() only stops in parallel, arguments that are a Collection. So this way, it blocks until the MessageSources are stopped, for only then, to trigger the stopping of the messageSink.

AbstractMessageProcessingPipelineSink might, or should, be stopped first, to stop acceptting new messages right away. With the current code, when AbstractPollingMessageSource is asked to stop, if there are ongoing polling tasks, if after some seconds these tasks receive new messages (up to pollTimeout), those messages will be emitted to the sink and be processed, when in fact we already know that we want to stop things.

Implement asynchronous stopping

In this idea, DefaultListenerContainerRegistry would implement the asynchronous stop command of SmartLifecycle (stop(Runnable callback)) so that the containers, and the blockings done to wait then to finish, would be made in parallel with the stopping of the embedded web container.

This would have the benefits of stopping the application faster, and to avoid issues with abrupt stoppings for instance with Kubernetes applications that for some corporate policy, needs to work with gracePeriod lesser than the default of 30s.

In this idea, the DefaultListenerContainerRegistry would have to have the same phase number as the one used by WebServerGracefulShutdownLifecycle. Although WebServerGracefulShutdownLifecycle exposes its phase through a public field, in practical terms I am not certain about making DefaultListenerContainerRegistry to refer explicitly to WebServerGracefulShutdownLifecycle.

[tomazfernandes]

Hey @jgslima, that sounds interesting!

Stop the MessageSink before

The motivation here was to have a as clean shutdown as possible by default. In case the listener has some simple processing such as persisting data that should not be a big issue, right?

Now, if we shutdown the sink first, any new messages retrieved by the source after that will be rejected, so what would be the point of keeping the source polls running? It would lead to messages being in flight for visibilityTimeout, which would prevent those messages from being consumed by other instances as well for that period, which might not be ideal too.

So maybe we can just shutdown all components in parallel, and do it in a way that cancels any polling tasks (which would lead to some exceptions showing up in the logs but we can probably work around that).

Either way, we should let the user configure the behavior they want, which would also make this not a breaking change. In the future we might consider changing the default.

What do you think, makes sense?

Implement asynchronous stopping

This would be an interesting counterpart to this other issue, which is about starting the containers asynchronously so the app can accept requests before container setup is complete.

TBH I'm not too familiar with the Spring phase logic - does it wait for all beans in one phase to stop before moving to the next phase? If you can please explain a bit further why we'd need to set the phase to the same as WebServerGracefulShutdownLifecycle

But we'd definitely not want to refer to any Spring Boot class from this project though, so we'd probably need to copy the value and have our own constant field in this project.

[jgslima]

Hello. Sorry for taking so long.

Yeah, what drives my concern is exactly to have a shutdown process as graceful as possible, to avoid loose database locks if the application is stopped abruptly. These locks cause some headaches. I even created an issue at spring-framework to increase the efficiency in the shutdown of ThreadPoolTaskExecutors when these are context beans (not really related with this discussion here). In Kubernetes, the default grace period is 30s, which may seem pretty enough, but if you have many components which stop() takes time to complete (like many executors which wait for the ongoing tasks to complete in a non-parallel way), you may be at risk. Also, in Kubernetes, usually the quicker the pod responds to a shutdown the better, because for Deployments with many pods, a version rollout may trigger an unecessary cluster autoscale just to handle for a moment the amount of new pods and the old pods still being stopped.

It would lead to messages being in flight for visibilityTimeout, which would prevent those messages from being consumed by other instances as well for that period.

It is a good point. In my case, I would usually prefer to just ignore the messages just taken from the queue, and wait for the visibilityTimeout to process them again, in order to be on the safe side regarding the graceful shutdown. But as a framework, it would be better for the user to choose this, as you said, through some configuration.

I do not think that AbstractPipelineMessageListenerContainer should stop the sources and the sink in parallel. The config should dictate which one of them is stopped first, because this way it is easier to reason about how things would work, and the user may make a clear choice. Also, we can assume that the sink (at least AbstractMessageProcessingPipelineSink) stops pretty immediately (it just changes an attribute to false and return) so if a new config is used to make the sink be the first argument to LifecycleHandler.stop(), I think it would work fine, for the cases where the user wants the application to stop quicker.

If you agree with the config and the idea, I may try to submit a PR in the next weekend.

Asynchronous stopping

Does it wait for all beans in one phase to stop before moving to the next phase?

As far as I remember, yes.
I will double check and get back to you.

[tomazfernandes]

Hey @jgslima, good stuff, nice work on the Spring Framework issue!

I think we got a reasonable approach there - we can have configurable order of components shutdown and as you pointed out the sink will be fairly immediate, so no need to stop them in parallel / indeterministic way.

Feel free to open an issue and PR for this.

For the Asynchronous Stopping, I still don't have a clear picture of what that would involve and what benefits it would bring to the table. I wonder if that work on Spring Framework 6.2 is something we could take advantage of here as well. But we can definitely discuss it further.

One point of improvement I know of that might be positively impacted by this is that if containers are declared as @Beans rather than @SqsListeners the shutdown process is managed by Spring and hence sequential.