Copilot commented on code in PR #7458:
URL: https://github.com/apache/ignite-3/pull/7458#discussion_r2715624820
##########
modules/network/src/main/java/org/apache/ignite/internal/network/netty/NettyServer.java:
##########
@@ -227,12 +253,22 @@ public CompletableFuture<Void> stop() {
return serverStartFuture.handle((unused, throwable) -> {
synchronized (startStopLock) {
+ List<CompletableFuture<Void>> closeFutures = new
ArrayList<>();
+
+ for (Channel acceptedChannel : acceptedChannels) {
Review Comment:
Potential ConcurrentModificationException: Iterating over acceptedChannels
while the closeFuture listeners (lines 219-223) can modify the same set
concurrently. When channels close asynchronously, their listeners will attempt
to remove elements from the set being iterated. Create a defensive copy before
iteration to avoid this race condition.
```suggestion
Set<Channel> acceptedChannelsSnapshot = new
HashSet<>(acceptedChannels);
for (Channel acceptedChannel : acceptedChannelsSnapshot)
{
```
##########
modules/network/src/main/java/org/apache/ignite/internal/network/netty/NettyServer.java:
##########
@@ -199,6 +209,22 @@ public void initChannel(SocketChannel ch) {
}
}
+ private void registerAcceptedChannelOrCloseIfStopped(SocketChannel ch) {
+ synchronized (startStopLock) {
+ if (stopped) {
+ ch.close();
Review Comment:
Channels accepted after stop() is called but before the server channel
closes are immediately closed (line 215) but their close operations are not
tracked in the stop() method's returned future. This could cause the stop
future to complete before these channels are fully closed. Consider tracking
these close futures as well to ensure complete cleanup.
```suggestion
CompletableFuture<Void> closeFuture =
toCompletableFuture(ch.close());
if (serverCloseFuture != null) {
serverCloseFuture =
CompletableFutures.allOf(List.of(serverCloseFuture, closeFuture));
} else {
serverCloseFuture = closeFuture;
}
```
##########
modules/network/src/main/java/org/apache/ignite/internal/network/netty/NettyServer.java:
##########
@@ -227,12 +253,22 @@ public CompletableFuture<Void> stop() {
return serverStartFuture.handle((unused, throwable) -> {
synchronized (startStopLock) {
+ List<CompletableFuture<Void>> closeFutures = new
ArrayList<>();
+
+ for (Channel acceptedChannel : acceptedChannels) {
+
closeFutures.add(toCompletableFuture(acceptedChannel.close()));
+ }
+
ServerChannel localChannel = channel;
if (localChannel != null) {
- localChannel.close();
+
closeFutures.add(toCompletableFuture(localChannel.close()));
+ }
+
+ if (serverCloseFuture != null) {
+ closeFutures.add(serverCloseFuture);
}
- return serverCloseFuture == null ?
CompletableFutures.<Void>nullCompletedFuture() : serverCloseFuture;
+ return CompletableFutures.allOf(closeFutures);
Review Comment:
The new behavior of explicitly closing accepted channels during server stop
lacks test coverage. Consider adding a test that verifies accepted channels are
properly closed when stop() is called, especially testing the scenario where
active client connections exist at the time of server shutdown.
##########
modules/network/src/main/java/org/apache/ignite/internal/network/netty/NettyServer.java:
##########
@@ -199,6 +209,22 @@ public void initChannel(SocketChannel ch) {
}
}
Review Comment:
Missing documentation for this private method. Consider adding a JavaDoc
comment explaining that this method either registers a newly accepted channel
for tracking or immediately closes it if the server has already been stopped.
```suggestion
/**
* Registers a newly accepted channel for tracking or closes it
immediately if the server has already been stopped.
*
* <p>If the server is running, the channel is added to the set of
accepted channels and a listener is attached to its
* close future to remove it from tracking when it is closed. If the
server is already stopped, the channel is closed
* right away and is not tracked.</p>
*
* @param ch Newly accepted Netty socket channel.
*/
```
--
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]
