shishkovilja commented on PR #13095:
URL: https://github.com/apache/ignite/pull/13095#issuecomment-4915358528
@anton-vinogradov , can you check race condition hypothesis?
## 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 |
--
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]
