Re: [PR] RATIS-2406. Fix resource cleanup bug in GrpcServicesImpl.closeImpl() method. [ratis]
slfan1989 commented on PR #1347: URL: https://github.com/apache/ratis/pull/1347#issuecomment-3900589769 > +1 the change looks good. Thank you very much for helping review the code! -- 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]
Re: [PR] RATIS-2406. Fix resource cleanup bug in GrpcServicesImpl.closeImpl() method. [ratis]
szetszwo merged PR #1347: URL: https://github.com/apache/ratis/pull/1347 -- 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]
Re: [PR] RATIS-2406. Fix resource cleanup bug in GrpcServicesImpl.closeImpl() method. [ratis]
slfan1989 commented on PR #1347:
URL: https://github.com/apache/ratis/pull/1347#issuecomment-3900511630
> @slfan1989 , thanks for the update!
>
> For simplicity, if there is an exception, let's just warn it but not throw
it. Then, we may try-catch each call. What do you think?
>
> ```java
> @Override
> public void closeImpl() {
> for (Server server : servers.values()) {
> server.shutdownNow();
> }
> boolean interrupted = false;
> for (Map.Entry server : servers.entrySet()) {
> try {
> server.getValue().awaitTermination();
> LOG.info("{}: Shutdown {} successfully", getId(), server.getKey());
> } catch (InterruptedException e) {
> Thread.currentThread().interrupt();
> LOG.warn("{}: Interrupted shutdown {}", getId(), server.getKey());
> interrupted = true;
> break;
> }
> }
>
> try {
> serverInterceptor.close();
> } catch (Exception e) {
> LOG.warn("{}: Failed to unregister metrics", getId(), e);
> }
>
> if (interrupted) {
> executor.shutdown(); // shutdown but not wait
> } else {
> ConcurrentUtils.shutdownAndWait(executor);
> }
>
> try {
> super.closeImpl();
> } catch (IOException e) {
> LOG.warn("{}: Failed to close proxies", getId(), e);
> }
> }
> ```
@szetszwo Thank you very much for reviewing the code! I agree with your
suggestions and have updated and optimized the code based on your feedback.
--
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]
Re: [PR] RATIS-2406. Fix resource cleanup bug in GrpcServicesImpl.closeImpl() method. [ratis]
szetszwo commented on code in PR #1347:
URL: https://github.com/apache/ratis/pull/1347#discussion_r2805236251
##
ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcServicesImpl.java:
##
@@ -342,22 +343,46 @@ public void startImpl() {
@Override
public void closeImpl() throws IOException {
-for (Map.Entry server : servers.entrySet()) {
- final String name = getId() + ": shutdown server " + server.getKey();
- LOG.info("{} now", name);
- final Server s = server.getValue().shutdownNow();
- super.closeImpl();
+InterruptedIOException interrupted = null;
+try {
+ for (Map.Entry server : servers.entrySet()) {
+final String name = getId() + ": shutdown server " + server.getKey();
+LOG.info("{} now", name);
Review Comment:
Sorry that I kept this log in my previous comment. Let's don't print it
since it is not useful.
--
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]
Re: [PR] RATIS-2406. Fix resource cleanup bug in GrpcServicesImpl.closeImpl() method. [ratis]
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 server : servers.entrySet()) {
> final String name = getId() + ": shutdown server " + server.getKey();
> LOG.info("{} now", name);
> server.getValue().shutdownNow();
> }
>
> InterruptedIOException interrupted = null;
> for (Map.Entry 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]
Re: [PR] RATIS-2406. Fix resource cleanup bug in GrpcServicesImpl.closeImpl() method. [ratis]
slfan1989 commented on PR #1347: URL: https://github.com/apache/ratis/pull/1347#issuecomment-3891467762 @szetszwo Could you please review this PR? Thank you very much! -- 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]
