prosgarz35 commented on PR #3198:
URL: https://github.com/apache/james-project/pull/3198#issuecomment-5807081423
### [IMAP] IDLE hardening: transport liveness checks, idempotent
cleanup, and reactive teardown
#### Context & Motivation
Following community review on IMAP IDLE stability, several edge cases
and race conditions were identified and addressed:
1. **False-positive disconnects on quiet IDLE sessions**:
Configuring `READER_IDLE` in Netty's `ImapIdleStateHandler` risked
disconnecting healthy RFC 2177 clients that remain silent while waiting for
server pushes and receiving server heartbeats.
2. **Double `popLineHandler()` race condition**:
Independent unregistration logic across the line handler, periodic
heartbeat failure handlers, and session errors could potentially invoke
`session.popLineHandler()` more than once, popping unintended downstream
handlers from
the session stack.
3. **Fire-and-forget `LOGOUT`**:
Invoking `.subscribe()` on `session.logout()` inside a continuation
line handler bypassed Reactor's backpressure and error propagation mechanisms.
4. **Stale resources on push delivery failures**:
When an unsolicited event update failed during write (e.g. broken
pipe / client abruptly severed connection), the error was logged, but IDLE
state and recurring heartbeat tasks could linger.
5. **Unselected IDLE completion**:
In `AUTHENTICATED` state without a selected mailbox (`sm == null`),
the readiness sink lacked an immediate completion signal.
#### Key Changes
- **Preserved `ALL_IDLE` in `ImapIdleStateHandler`**:
Reverted to standard `ALL_IDLE` handling to ensure valid, silent IDLE
sessions receiving heartbeats are never terminated prematurely. Connection
liveness is instead managed cleanly via `channel.isActive()`.
- **Introduced `ImapSession#isConnected()`**:
Added default non-breaking method `isConnected()` to `ImapSession`
(backed by `channel.isActive()` in `NettyImapSession`). Used by `IdleProcessor`
to abort heartbeats and continuation processing on dropped connections.
- **Idempotent Atomic Cleanup (`cleanupIdle`)**:
Consolidated mailbox listener unregistration and
`session.popLineHandler()` inside `cleanupIdle(...)` guarded by an atomic CAS
(`idleActive.compareAndSet(true, false)`). Ensures teardown runs strictly once
across concurrent
`DONE`, timeout, push failure, or error paths.
- **Managed Reactive Pipeline for mid-IDLE `LOGOUT`**:
Continuation line handler returns `session1.logout()` directly as part
of the `Mono<Void>` pipeline rather than calling detached `.subscribe()`.
- **Immediate Push Error Teardown**:
`IdleMailboxListener#reactiveEvent` now triggers `cleanupIdle(...)`
upon write errors, immediately halting heartbeats and removing stale listeners.
- **Immediate Readiness for Unselected IDLE**:
Added immediate `idleReadySink.tryEmitEmpty()` when `sm == null` (RFC
2177 unselected IDLE).
#### Verification
- Backward compatibility: 100% preserved (all public APIs intact,
default methods provide seamless interop).
- Modules `:protocols-imap` and `:james-server-protocols-imap4` compile
cleanly with Maven 3.9+ and OpenJDK.
- Checkstyle: **0 violations**.
--
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]
