RATIS-2406. Fix resource cleanup bug in GrpcServicesImpl.closeImpl() method.

[slfan1989]

What changes were proposed in this pull request?

Fixed a resource cleanup bug in GrpcServicesImpl.closeImpl() method where:

1. Multiple invocations bug: super.closeImpl() was called inside the server shutdown loop, causing the parent class cleanup logic to be executed multiple times (once per server entry in the map)

2. Resource leak risk: If any server shutdown fails with an exception, super.closeImpl(), serverInterceptor.close(), and ConcurrentUtils.shutdownAndWait(executor) would not be executed, leading to resource leaks

Changes made:

• Wrapped the server shutdown loop in a try-finally block
• Moved super.closeImpl() to the finally block to ensure it executes exactly once
• Ensured proper cleanup order: child resources → parent resources → remaining resources

What is the link to the Apache JIRA

RATIS-2406. Fix resource cleanup bug in GrpcServicesImpl.closeImpl() method.

How was this patch tested?

• Existing unit tests pass

[slfan1989]

@szetszwo Could you please review this PR? Thank you very much!

[szetszwo]

@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.

  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();
  }

[slfan1989]

@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.

  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!