Copilot commented on code in PR #7926:
URL: https://github.com/apache/ignite-3/pull/7926#discussion_r3032028770
##########
modules/client-common/src/main/java/org/apache/ignite/internal/client/proto/ClientMessageDecoder.java:
##########
@@ -43,13 +44,19 @@ public class ClientMessageDecoder extends
LengthFieldBasedFrameDecoder {
*/
public ClientMessageDecoder() {
super(Integer.MAX_VALUE - HEADER_SIZE, 0, HEADER_SIZE, 0, HEADER_SIZE,
true);
+
+ // Effectively disable automatic calls to discardReadBytes /
discardSomeReadBytes:
+ // We pass the buffers to other threads, and discardReadBytes modifies
the buffer concurrently,
+ // leading to race conditions and corrupted offsets.
+ // Moreover, discardReadBytes is not very beneficial with our access
patterns:
+ // Read small header => pass to another thread => read fully =>
discard.
+ setDiscardAfterReads(Integer.MAX_VALUE);
Review Comment:
`setDiscardAfterReads(Integer.MAX_VALUE)` only postpones Netty’s automatic
`discard(Read|SomeRead)Bytes` calls; after enough decode cycles it can still
run and reintroduce the same cross-thread corruption you’re trying to
eliminate. If the intent is to fully disable discarding, use an explicit
“disabled” setting supported by Netty (or another deterministic mechanism)
rather than a large finite threshold.
```suggestion
// Disable automatic calls to discardReadBytes /
discardSomeReadBytes:
// We pass the buffers to other threads, and discardReadBytes
modifies the buffer concurrently,
// leading to race conditions and corrupted offsets.
// Moreover, discardReadBytes is not very beneficial with our access
patterns:
// Read small header => pass to another thread => read fully =>
discard.
setDiscardAfterReads(0);
```
##########
modules/client-common/src/main/java/org/apache/ignite/internal/client/proto/ClientMessageDecoder.java:
##########
@@ -43,13 +44,19 @@ public class ClientMessageDecoder extends
LengthFieldBasedFrameDecoder {
*/
public ClientMessageDecoder() {
super(Integer.MAX_VALUE - HEADER_SIZE, 0, HEADER_SIZE, 0, HEADER_SIZE,
true);
+
+ // Effectively disable automatic calls to discardReadBytes /
discardSomeReadBytes:
+ // We pass the buffers to other threads, and discardReadBytes modifies
the buffer concurrently,
+ // leading to race conditions and corrupted offsets.
+ // Moreover, discardReadBytes is not very beneficial with our access
patterns:
+ // Read small header => pass to another thread => read fully =>
discard.
+ setDiscardAfterReads(Integer.MAX_VALUE);
Review Comment:
Disabling cumulation discarding can increase memory usage and long-lived
index growth: read bytes at the beginning of the cumulation buffer won’t be
reclaimed for future reads, so the buffer may expand/copy more often than
necessary under sustained traffic. Consider an alternative that avoids sharing
mutable cumulation state across threads (e.g., copying/detaching the extracted
frame before offloading) so Netty can keep discarding safely.
--
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]
