prosgarz35 commented on code in PR #3198:
URL: https://github.com/apache/james-project/pull/3198#discussion_r4088074963
##########
server/protocols/protocols-imap4/src/main/java/org/apache/james/imapserver/netty/ImapIdleStateHandler.java:
##########
@@ -42,7 +42,7 @@ public class ImapIdleStateHandler extends IdleStateHandler
implements NettyConst
private static final Logger LOGGER =
LoggerFactory.getLogger(ImapIdleStateHandler.class);
public ImapIdleStateHandler(int allIdleTimeSeconds) {
- this(0, 0, allIdleTimeSeconds);
+ this(allIdleTimeSeconds, 0, allIdleTimeSeconds);
Review Comment:
# Why `allIdleTimeSeconds` Alone Is Not Enough in `ImapIdleStateHandler`
## 1. Executive Summary
In Netty-based servers, relying solely on `IdleState.ALL_IDLE`
(`allIdleTimeSeconds`) to detect dropped client connections fails whenever the
server generates outbound traffic on its own.
In Apache James, when IMAP IDLE heartbeats (`* OK Still here`) are enabled,
each outgoing heartbeat updates the channel's `lastWriteTime`. This
**continuously resets the `ALL_IDLE` timer**, preventing Netty from ever timing
out silently dropped client connections (half-open sockets). Adding
`IdleState.READER_IDLE` (`readerIdleTimeSeconds`) fixes this flaw.
---
## 2. Netty's `IdleStateHandler` Internals
Netty's `IdleStateHandler` tracks two distinct timestamps on each channel
pipeline:
- `lastReadTime`: Timestamp of the last inbound byte received from the
remote peer.
- `lastWriteTime`: Timestamp of the last outbound byte successfully flushed
to the socket.
From these timestamps, Netty evaluates three distinct idle events:
| Netty State | Definition | Evaluation Condition |
| :--- | :--- | :--- |
| **`READER_IDLE`** | No inbound data received from client | `now -
lastReadTime >= readerIdleTime` |
| **`WRITER_IDLE`** | No outbound data sent to client | `now - lastWriteTime
>= writerIdleTime` |
| **`ALL_IDLE`** | Neither read **NOR** write occurred | `now -
max(lastReadTime, lastWriteTime) >= allIdleTime` |
Notice the mathematical definition of `ALL_IDLE`:
$$\text{idle} = \text{now} - \max(\text{lastReadTime}, \text{lastWriteTime})
\ge \text{allIdleTime}$$
For `ALL_IDLE` to trigger, the connection must be completely silent in
**both directions simultaneously**.
---
## 3. The Failure Mechanism with Heartbeats
1. A client connects, selects a mailbox, and enters `IDLE`.
2. When James is configured with `enableIdle = true` (server heartbeats
enabled), `IdleProcessor` schedules a periodic heartbeat task:
```java
StatusResponse response =
getStatusResponseFactory().untaggedOk(HumanReadableText.HEARTBEAT);
responder.respond(response);
responder.flush();
```
This sends `* OK Still here\r\n` every **N minutes** (typically every 2
to 15 minutes, well before the 30-minute server timeout).
3. **Every time James writes this heartbeat, Netty sets `lastWriteTime =
now`.**
4. Because `lastWriteTime` is refreshed every few minutes:
$$\max(\text{lastReadTime}, \text{lastWriteTime}) = \text{timestamp of
last outgoing heartbeat}$$
The elapsed duration `now - lastWriteTime` will **never exceed the
heartbeat interval (e.g. 15 minutes)**.
5. If the server timeout is configured to 30 minutes (1800s), **`ALL_IDLE`
will NEVER fire**.
---
## 4. Real-World Scenario: The "Ghost" Connection
Consider a mobile email client (iOS Mail, Android K-9, etc.) or a laptop
entering sleep mode:
1. The device loses network connectivity abruptly (cell tower handoff drop,
tunnel, Wi-Fi loss, or NAT table expiration) without sending a clean `TCP FIN`
or `TCP RST`.
2. From the operating system and Netty channel perspective, the socket
remains in the `ESTABLISHED` state (a classic **half-open TCP connection**).
3. The client is dead and will **never send any data again** (`lastReadTime`
stops advancing).
### Outcome Without `READER_IDLE`:
- Every 15 minutes, James writes `* OK Still here` into the Netty socket
buffer.
- Because kernel TCP send buffers absorb the small write locally, Netty
marks the write successful and resets `lastWriteTime`.
- `ALL_IDLE` is perpetually postponed.
- The connection remains alive on the server for **hours or days**,
retaining:
- An open OS file descriptor.
- Netty channel buffers and pipeline handlers.
- The `ImapSession` and `SelectedMailboxImpl` state with active event
listeners.
### Outcome With `READER_IDLE`:
- RFC 2177 requires IMAP clients to refresh or re-issue `IDLE` at least once
every 29 minutes.
- When the client produces no inbound traffic for 30 minutes, `now -
lastReadTime >= readerIdleTime` evaluates to `true`.
- Netty fires `READER_IDLE`.
- `ImapIdleStateHandler.channelIdle(...)` triggers `session.logout()` and
`ctx.channel().close()`, immediately freeing all resources.
---
## 5. Code Comparison
### Before
```java
// Constructor only configured allIdleTimeSeconds:
public ImapIdleStateHandler(int allIdleTimeSeconds) {
this(0, 0, allIdleTimeSeconds);
}
// Handler only listened for ALL_IDLE:
if (e.state().equals(IdleState.ALL_IDLE)) {
// Logout and close...
}
```
### After
```java
// Configures readerIdleTimeSeconds equal to allIdleTimeSeconds:
public ImapIdleStateHandler(int allIdleTimeSeconds) {
this(allIdleTimeSeconds, 0, allIdleTimeSeconds);
}
// Handler triggers on either ALL_IDLE or READER_IDLE:
if (e.state().equals(IdleState.ALL_IDLE) ||
e.state().equals(IdleState.READER_IDLE)) {
// Logout and close...
}
```
--
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]
