jon-valliere commented on PR #77:
URL: https://github.com/apache/mina/pull/77#issuecomment-6062446439
I had Claude look into it and provided some background.
**Root cause / history**
The `synchronized (queue)` blocks around the drain loops were part of the
original G1 design. From the description of #44:
> The code which pulls objects out of this Queue is blocked by itself so no
two threads can be pulling from the Queue concurrently.
They were commented out in 62643fece ("alright lets remove all the
synchronization from the queue flushes"). That was an experiment on the
in-progress `bugfix/DIRMINA-1173` branch. I never meant it to be merged. The
branch was later merged into `2.2.X` together with unrelated maintenance work,
so the experiment shipped in 2.2.4. This PR restores the intended design.
**Why `mWriteQueue` is the one that matters**
Encryption happens under the handler monitor, so records go into
`mWriteQueue` in the correct order. The drain runs after the monitor is
released, though, and it's reached from several threads at once:
- the application thread: `write()` → `forward_writes`
- the IO processor thread: `ack()` → `flush_start` (which encrypts more
queued chunks because of `MAX_UNACK_MESSAGES`) → `forward_writes`
- the receive path: `receive()` → `forward_writes`
If two threads poll consecutive records and reach `filterWrite` in the
opposite order, the records go out of order on the wire. TLS sequence numbers
are implicit, so the peer fails with `bad_record_mac` or corrupted data. Large
messages split into several records make this much more likely. That fits
@elecharny's observation that locking `mWriteQueue` alone makes the test pass.
`mReceiveQueue` needs the same protection. `receive()` normally runs on one
IO thread per session, but a thread-dispatching filter can be placed in front
of `SslFilter`. Decryption in `receive_start` is serialized by the handler
monitor, but `forward_received` drains after that monitor is released. Without
the lock, one thread can still be delivering its decrypted buffers while
another decrypts the next message and drains too, and the application receives
the plaintext out of order. `mEventQueue` is exposed the same way. All three
locks belong to the original design and should stay.
**Requested changes**
1. **Debug logging:** please drop the new entry logs in `forward_*()`. They
add 2–3 lines per event and don't say much. The existing per-item logs are
enough.
2. **`SslFilterTest`:**
- The 61000–80000 loop is ~19k round trips, which is too slow for the
regular build. A handful of sizes around the record/packet boundaries should be
enough.
- `CompareFilter` only logs on a data mismatch and then counts down the
latch, so corrupted data doesn't fail the test. Please record it as a failure
(e.g. set `failure`) so the assertion catches it.
- Progress is logged at `ERROR` level; please use `DEBUG`.
- Missing newline at end of file.
3. **`SslEnd2EndTest`** is bidirectional and asserts the content, which
covers what was asked for earlier. If it reproduces the failure reliably
without the fix, I'd be fine keeping only this one.
+1 on the fix itself once the tests are cleaned up.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
