anton-vinogradov commented on PR #13095:
URL: https://github.com/apache/ignite/pull/13095#issuecomment-4916406432
> @anton-vinogradov , can you check race condition hypothesis, please?
>
> ## Race Condition in GridIoManager
> ### 1. Architecture Context
> **Two-level message processing system:**
>
> * **`GridIoManager`** - Generic communication manager (lower level)
> * **`GridCacheIoManager`** - Cache-specific message manager (upper level)
>
> **Message flow:**
>
> ```
> Remote Node → NIO Thread → TcpCommunicationSpi → GridNioServer
> → GridNioFilterChain
> →
GridNioServerListener.onMessage()
> →
GridIoManager.commLsnr.onMessage()
> → GridIoManager.onMessage0()
> ↓
> (GridCacheIoManager listener for
TOPIC_CACHE)
> ↓
> GridCacheIoManager.onMessage0()
> → unmarshall() with deployment
classloader
> ↓
> Worker Thread →
processRegularMessage0()
> → unmarshalPayload() →
invokeListener()
> ```
>
> ### 2. Problematic Code Sections
> #### Section A: GridIoManager.onMessage0() - unmarshalNio on NIO Thread
> ```java
> // Line ~1262: unmarshalNio called on NIO thread
> MessageMarshaller.unmarshalNio(ctx.messageFactory(), msg, ctx);
> ```
>
> **What it does:**
>
> * Unmarshals only `@NioField` annotated fields
> * Uses `kctx.messageFactory()` and `kctx` (configuration context)
> * Called for ALL messages, including `GridCacheMessage`
>
> #### Section B: GridCacheIoManager.unmarshall() - Full unmarshal with
deployment classloader
> ```java
> private void unmarshall(UUID nodeId, GridCacheMessage cacheMsg) {
> // Lines ~1544-1566: Full unmarshal with deployment classloader
> MessageMarshaller.unmarshal(cctx.kernalContext().messageFactory(),
> cacheMsg, cctx.kernalContext(), null,
cctx.deploy().globalLoader());
> }
> ```
>
> **What it does:**
>
> * Full unmarshal for `GridCacheMessage`
> * Uses `cctx.deploy().globalLoader()` (deployment classloader)
>
> #### Section C: GridCacheIoManager - @NioField Field Handling
> From `MessageMarshallerGenerator.java`:
>
> ```java
> /** Cache-free unmarshal of a {@code @NioField} message field on the NIO
thread (no cache context available). */
> private List<String> unmarshalNioField(String accessor) {
> List<String> code = new ArrayList<>();
>
> code.add(indentedLine("if (%s != null)", accessor));
>
> indent++;
>
>
code.add(indentedLine("MessageMarshaller.unmarshal(kctx.messageFactory(), %s,
kctx);", accessor));
>
> indent--;
>
> return code;
> }
> ```
>
> **What it does:**
>
> * For `@NioField` fields, generates code that calls
`MessageMarshaller.unmarshal()`
> * But this is called from `unmarshalNio()` which uses `kctx` (not `cctx`)
> * **Problem**: The nested message's marshaller may need `cctx` (cache
context)!
>
> ### 3. Race Condition Scenarios
> #### Scenario 1: Normal Flow (No Issue)
> ```
> 1. NIO Thread: GridIoManager.onMessage0() → unmarshalNio()
> → unmarshals @NioField fields (e.g., topicMsg) with kctx
>
> 2. Worker Thread: processRegularMessage0() → unmarshalPayload()
> → listenerGet0() returns GridCacheIoManager listener
> → GridCacheIoManager.onMessage() → unmarshall()
> → Full unmarshal with deployment classloader
> ```
>
> #### Scenario 2: Delayed Messages Replay (RACE CONDITION)
> When messages arrive before `started = true`:
>
> ```
> 1. NIO Thread receives message
> started = false
> Message added to waitMap (line ~1257)
> Return early, NO unmarshalNio yet
>
> 2. Node joins event triggers replay
> waitMap messages are replayed via commLsnr.onMessage() (line ~1038)
>
> 3. NIO Thread: onMessage0() called FIRST time
> started = true now
> unmarshalNio() called on NIO thread
> @NioField fields get unmarshalled with kctx
>
> 4. Worker Thread (from delayed execution):
> processRegularMessage0() → listenerGet0() returns GridCacheIoManager
listener
> GridCacheIoManager.onMessage() → unmarshall()
> Full unmarshal with deployment classloader
> ```
>
> #### Scenario 3: Cross-thread Access to @NioField Fields
> For messages like `GridIoMessage` with `@NioField topicMsg`:
>
> ```java
> public class GridIoMessage implements Message, SpanTransport {
> @NioField
> @Order(1)
> GridTopicMessage topicMsg;
> ...
> }
> ```
>
> **Problem:**
>
> * `GridNioServer` calls `GridNioServerListener.onMessage()` directly
> * `GridIoManager` implements `GridNioServerListener` as `commLsnr`
> * `commLsnr.onMessage()` → `onMessage0()` → `unmarshalNio()`
> * **No busy lock protection** between NIO and worker threads
> * Same message object accessed by multiple threads concurrently
>
> #### Scenario 4: Nested @NioField Messages
> If `GridCacheMessage` has `@NioField` fields:
>
> ```java
> public class SomeCacheMessage extends GridCacheMessage {
> @NioField
> @Order(10)
> AnotherMessage nested; // This message may need cache context!
> }
> ```
>
> Generated code (from `MessageMarshallerGenerator.java`):
>
> ```java
> private List<String> unmarshalNioField(String accessor) {
> List<String> code = new ArrayList<>();
> code.add(indentedLine("if (%s != null)", accessor));
> indent++;
>
code.add(indentedLine("MessageMarshaller.unmarshal(kctx.messageFactory(), %s,
kctx);", accessor));
> indent--;
> return code;
> }
> ```
>
> **Problem:**
>
> * `kctx` = `GridKernalContext` (configuration context)
> * `cctx` = `GridCacheSharedContext` (cache context with deployment loader)
> * Nested message's marshaller may need `cctx` but gets only `kctx`!
>
> ### 4. Root Causes
> Issue Location Problem
> **1. No busy lock in unmarshalNio** GridIoManager.onMessage0:~1262
Same message accessed by NIO and worker threads without synchronization
> **2. Wrong context in nested @NioField** MessageMarshallerGenerator:~263
Uses `kctx` instead of `cctx` for nested messages
> **3. No protection for delayed replay** GridIoManager.onMessage0
Delayed messages call unmarshalNio multiple times
> **4. unmarshalPayload early return** GridIoManager:~1479-1481
Assumes GridCacheMessage already unmarshalled, but timing is racey
> ### 5. Recommendations
> #### Fix 1: Add Busy Lock to unmarshalNio
> ```java
> // In GridIoManager.onMessage0:
> try {
> if (stopping) {
> return;
> }
>
> if (!busyLock0.tryReadLock()) {
> return;
> }
>
> try {
> if (stopping) {
> return;
> }
>
> MessageMarshaller.unmarshalNio(ctx.messageFactory(), msg, ctx);
>
> // ... rest of processing
> } finally {
> busyLock0.unlockRead();
> }
> }
> ```
>
> #### Fix 2: Use Correct Context for Nested @NioField Messages
> The `unmarshalNioField` method needs to receive the correct context:
>
> ```java
> private List<String> unmarshalNioField(String accessor, String ctxVar) {
> List<String> code = new ArrayList<>();
> code.add(indentedLine("if (%s != null)", accessor));
> indent++;
>
code.add(indentedLine("MessageMarshaller.unmarshal(kctx.messageFactory(), %s,
%s);", accessor, ctxVar));
> indent--;
> return code;
> }
> ```
>
> #### Fix 3: Add Synchronization for @NioField Fields
> ```java
> static <M extends Message> void unmarshalNio(MessageFactory factory, M
msg, GridKernalContext kctx)
> throws IgniteCheckedException {
> MessageMarshaller<M> m = resolve(factory, msg);
> if (m != null) {
> synchronized(msg) {
> m.unmarshalNio(msg, kctx);
> }
> }
> }
> ```
>
> #### Fix 4: Add "Already Unmarshalled" Marker
> ```java
> // In GridIoMessage:
> private volatile boolean nioUnmarshalled;
>
> public boolean isNioUnmarshalled() {
> return nioUnmarshalled;
> }
>
> public void setNioUnmarshalled() {
> this.nioUnmarshalled = true;
> }
>
> // In GridIoManager.unmarshalNio:
> if (!msg.isNioUnmarshalled()) {
> msg.setNioUnmarshalled();
> m.unmarshalNio(msg, kctx);
> }
> ```
>
> ### 6. Summary
> Category Details
> **Severity** High
> **Root Cause** Missing busy lock protection and incorrect context for
@NioField fields
> **Impact** Corrupted deserialization, race conditions, classloader
incompatibility
> **Required Fix** Add busy lock protection, fix context, add
synchronization
Following up on point 2/scenario 4 — I've added a compile-time guard for it:
@NioField is now rejected by the message processor not only on non-Message
fields, but also on any Message-typed field whose marshaller needs a cache
object context (i.e. carries CacheObject/cache-aware @Order fields). Since
@NioField unmarshalling runs on the NIO thread with no cache context, such a
field could otherwise be silently left un-unmarshalled — the guard makes that a
compilation error, so the "nested @NioField message needs cctx" case is ruled
out by construction. Covered by a negative test
(testNioFieldNeedingCacheContextFails); the only production @NioField field,
GridIoMessage.topicMsg (a context-free GridTopicMessage), still compiles.
The other three points hold as analyzed above — unmarshalNio already runs
under the busyLock read lock, sits below the waitMap gate (so it runs exactly
once, and GridTopicMessage.unmarshal is idempotent anyway), and the NIO→worker
step is a happens-before hand-off via the pool submit, not concurrent access.
MessageUnmarshalDedup guards against any double unmarshal across the whole
suite.
--
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]
