slfan1989 commented on PR #1347:
URL: https://github.com/apache/ratis/pull/1347#issuecomment-3893861883
> @slfan1989 , thanks for working on this!
>
> Good catch on the bug. Some suggestions:
>
> * Let shutdown all the servers first and then awaitTermination afterward.
> * If it is interrupted, shutdown executor but not wait.
> * serverInterceptor.close() won't throw exception, so we could just call
it.
> * Move super.closeImpl() to the end.
>
> ```java
> public void closeImpl() throws IOException {
> for (Map.Entry<String, Server> server : servers.entrySet()) {
> final String name = getId() + ": shutdown server " + server.getKey();
> LOG.info("{} now", name);
> server.getValue().shutdownNow();
> }
>
> InterruptedIOException interrupted = null;
> for (Map.Entry<String, Server> server : servers.entrySet()) {
> final String name = getId() + ": shutdown server " + server.getKey();
> try {
> server.getValue().awaitTermination();
> } catch (InterruptedException e) {
> Thread.currentThread().interrupt();
> interrupted = IOUtils.toInterruptedIOException(name + "
interrupted", e);
> }
> LOG.info("{} successfully", name);
> }
>
> serverInterceptor.close();
>
> if (interrupted != null) {
> executor.shutdown(); // shutdown but not wait
> throw interrupted;
> }
>
> ConcurrentUtils.shutdownAndWait(executor);
> super.closeImpl();
> }
> ```
@szetszwo Thank you very much for your review!
I made a few improvements based on your code.
- super.closeImpl() is executed in the finally block.
- When InterruptedException occurs, a WARN log is printed for
troubleshooting.
- In the current implementation, if an interruption occurs and
super.closeImpl() does not throw an exception,
the interrupted exception will still be thrown earlier and will not be
lost.
When you have a chance, could you take another look at this PR? Thanks a lot!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
