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

Add a build method to ServerListener responsible for graceful termination of an external Executor. #5087

Merged

Conversation

tomatophobia
Copy link
Contributor

Motivation:

  • Related Add a builder for ServerListener #5031
  • Users may create a dedicated ExecutorService to handle background tasks. In such cases, users may want to gracefully shut down the ExecutorService in sync with Armeria's shutdown. To facilitate this, it would be useful to add an API that builds a ServerListener responsible for graceful termination. This ServerListener would be able to handle the graceful shutdown of the user's custom ExecutorService.

Modifications:

  • Add stoppingWithExecutor methods to ServerListenerBuilder.
  • Extend ShutdownSupport#of(ExecutorService, ...) methods with time-limited termination and customized termination logic.
  • Add a factory method ServerListner#withExecutor for creating a complete ServerListener.
  • Add tests for ServerListener.

Result:

  • Closes Add a builder for ServerListener #5031
  • Users can easily create and utilize a ServerListener that synchronizes the graceful shutdown of their own custom ExecutorService with Armeria's shutdown.

1. Add `withExecutor` method to `ServerListener`.
2. Add `stoppingWithExecutor` method to `ServerListenerBuilder`.
3. Extended `ShutdownSupport` methods with time-limited termination and customized termination logic.
4. Added tests for `ServerListener`.
@tomatophobia tomatophobia force-pushed the add-shutdown-builder-for-server-listener branch from bea5b90 to cec892b Compare July 31, 2023 10:42
Copy link
Contributor Author

@tomatophobia tomatophobia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have left a review while extending the existing ShutdownSupport#of method. I would appreciate it if you could check the following points.

FYI, I implemented the graceful shutdown logic based on the ExecutorService's javadoc.

Copy link
Contributor Author

@tomatophobia tomatophobia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a total of 6 tests, divided into three types of Executor, depending on whether they wait infinitely or with a finite period:

  1. Executor that runs periodically every 1 second.
  2. Executor that runs an infinite loop and can be interrupted.
  3. Executor that runs an infinite loop and cannot be interrupted.

@codecov
Copy link

codecov bot commented Aug 5, 2023

Codecov Report

Attention: 18 lines in your changes are missing coverage. Please review.

Comparison is base (a8a9ca2) 0.00% compared to head (e1733e2) 73.94%.

Files Patch % Lines
...a/com/linecorp/armeria/server/ShutdownSupport.java 54.54% 6 Missing and 4 partials ⚠️
...linecorp/armeria/server/ServerListenerBuilder.java 50.00% 8 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #5087       +/-   ##
===========================================
+ Coverage        0   73.94%   +73.94%     
- Complexity      0    20103    +20103     
===========================================
  Files           0     1729     +1729     
  Lines           0    74146    +74146     
  Branches        0     9465     +9465     
===========================================
+ Hits            0    54830    +54830     
- Misses          0    14834    +14834     
- Partials        0     4482     +4482     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ikhoon ikhoon added this to the 1.26.0 milestone Aug 21, 2023
1. Apply the `@UnstableApi` annotation.
2. Remove the method that receiving customized shutdown logic.
3. Modify the method to accept `terminationTimeoutMillis` as a parameter.
4. Delete `UnstoppableScheduledExecutorService`.
Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall 👍 Left some minor comments/questions

* is stopping. It will wait indefinitely for the {@link ExecutorService} to terminate during shutdown.
*/
@UnstableApi
public ServerListenerBuilder shutdownWhenStopping(ExecutorService executorService) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question) This is more a question to @ikhoon since the issue seems to suggest this.
AFAIK whenStopping callbacks are invoked before the server starts shutting down.

If the ExecutorService is used by the server, I fear that an early cancellation may result in rejected tasks -> failed requests.
Is there a reason we're not adding to whenStopped instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I think both shutdownWhenStopping() and shutdownWhenStopped() are useful.

  • If an executor is used for periodic jobs such as updating a server's health, an early stop would be preferred for fast-rolling updates.
  • If an executor is used for async execution of incoming requests, lazy stopping would make sense.

How about adding both? :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds like a good idea. I've also added shutdownWhenStopped().

@tomatophobia tomatophobia force-pushed the add-shutdown-builder-for-server-listener branch from ca8dea4 to 675edb4 Compare September 9, 2023 06:29
* is stopping. It will wait indefinitely for the {@link ExecutorService} to terminate during shutdown.
*/
@UnstableApi
public ServerListenerBuilder shutdownWhenStopping(ExecutorService executorService) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I think both shutdownWhenStopping() and shutdownWhenStopped() are useful.

  • If an executor is used for periodic jobs such as updating a server's health, an early stop would be preferred for fast-rolling updates.
  • If an executor is used for async execution of incoming requests, lazy stopping would make sense.

How about adding both? :-)

Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall. Left some suggestions. 😉

.build();
server.start().get();
stopFuture = server.stop();
Thread.sleep(5000);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you reduce these values so that the amount of total build time isn't increased too much?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I combined the 12 divided tests into one to reduce the total test execution time. The tests, which used to take 60 seconds to complete, have now been reduced to 20 seconds.

Note: I attempted to terminate the ExecutorService instances concurrently without waiting for them to shut down sequentially. However, due to Armeria's ServerListener waiting for the synchronous termination of registered listeners, I had no choice but to wait for Executor termination one by one.

@ikhoon ikhoon modified the milestones: 1.26.0, 1.27.0 Oct 18, 2023
@tomatophobia tomatophobia force-pushed the add-shutdown-builder-for-server-listener branch from 05f4471 to 3d58754 Compare October 19, 2023 16:37
Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Thanks! 👍

Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks @tomatophobia 🙇 👍 🙇

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @tomatophobia! 🙇‍♂️🙇‍♂️

@ikhoon ikhoon merged commit 550c10b into line:main Jan 24, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a builder for ServerListener
4 participants