duongkame commented on code in PR #1021:
URL: https://github.com/apache/ratis/pull/1021#discussion_r1458167497
##########
ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcClientProtocolService.java:
##########
@@ -363,6 +367,7 @@ void processClientRequest(PendingOrderedRequest pending) {
final long seq = pending.getSeqNum();
processClientRequest(pending.getRequestRef(),
reply -> slidingWindow.receiveReply(seq, reply, this::sendReply));
+ pending.release();
Review Comment:
The `processClientRequest` eventually calls
`submitClientRequestAsync(ReferenceCountedObject<RaftClientRequest>)` that
returns a Future.
I realized that when calling any asynchronous method accepting a reference
counter, we hardly need to wait for `whenComplete` to release the counter. Look
at an example below
```
final RaftClientRequest request = requestRef.retain();
try {
// do something with the request.
// call asynchronous
return submitClientRequestAsync()
// We don't need and should not do this
// .whenComplete((r, e) -> requestRef.release());
;
} finally {
// instead just release the counter here.
requestRef.release()
}
```
This is because:
1. Inside `submitClientRequestAsync`, there's already `retain` when needed
(in LogSegment cache). Other words, if the internal logic of
`submitClientRequestAsync` needs to extend the life of the referenced object,
that should be done there. The outside client code responsibility is to ensure
the reference is `retainable` when it reaches the `submitClientRequestAsync`
and this is done by the outside `retain`. The outside code releases the counter
as soon as it finishes using it.
2. There're cases that `whenComplete` is not called. For example, when
client times out or disconects, the return future seems to be discarded
(documented per RATIS-2007).
--
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]
