Copilot commented on code in PR #6512:
URL: https://github.com/apache/ignite-3/pull/6512#discussion_r2309929735
##########
modules/client/src/main/java/org/apache/ignite/internal/client/TcpClientChannel.java:
##########
@@ -446,7 +443,7 @@ private <T> CompletableFuture<T> send(
@Nullable CompletableFuture<PayloadInputChannel> notificationFut,
ClientMessageUnpacker unpacker
) {
- try (unpacker) {
+ try {
if (payloadReader != null) {
return payloadReader.apply(new PayloadInputChannel(this,
unpacker, notificationFut));
}
Review Comment:
The unpacker resource is no longer automatically closed in the complete()
method after removing the try-with-resources statement. This could lead to
resource leaks if an exception occurs during payloadReader.apply() execution.
##########
modules/client/src/main/java/org/apache/ignite/internal/client/TcpClientChannel.java:
##########
@@ -823,17 +822,18 @@ private void completeNotificationFuture(
// Add reference count before jumping onto another thread.
unpacker.retain();
- asyncContinuationExecutor.execute(() -> {
- try {
- if (!fut.complete(new PayloadInputChannel(this, unpacker,
null))) {
- unpacker.close();
+ try {
+ asyncContinuationExecutor.execute(() -> {
+ try (unpacker) {
Review Comment:
The try-with-resources block will close the unpacker immediately after
creating the PayloadInputChannel, but the PayloadInputChannel likely needs the
unpacker to remain open for reading data. This will cause the channel to fail
when attempting to read from a closed unpacker.
```suggestion
try {
```
##########
modules/client/src/main/java/org/apache/ignite/internal/client/TcpClientChannel.java:
##########
@@ -797,17 +794,19 @@ public String endpoint() {
return cfg.getAddress().toString();
}
- private static void
completeRequestFuture(CompletableFuture<ClientMessageUnpacker> fut,
ClientMessageUnpacker unpacker) {
- // Add reference count before jumping onto another thread (due to
handleAsync() in send()).
+ private void
completeRequestFutureAsync(CompletableFuture<ClientMessageUnpacker> fut,
ClientMessageUnpacker unpacker) {
+ // Add reference count before jumping onto another thread.
unpacker.retain();
try {
- if (!fut.complete(unpacker)) {
- unpacker.close();
- }
- } catch (Throwable t) {
+ asyncContinuationExecutor.execute(() -> {
+ try (unpacker) {
+ fut.complete(unpacker);
+ }
Review Comment:
Using try-with-resources here will close the unpacker immediately after
completing the future, but the unpacker may still be needed by the future's
result consumer. This creates a race condition where the unpacker could be
closed before it's actually used.
--
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]
