prosgarz35 commented on PR #3198:
URL: https://github.com/apache/james-project/pull/3198#issuecomment-5834073020
# Pull Request: [IMPROVEMENT] Harden IMAP IDLE lifecycle and continuation
handling
## PR Description / Comment Text
### Motivation & Context
This PR hardens the IMAP IDLE command lifecycle, concurrency handling, and
error resilience following maintainer feedback (@chibenwa, @quantranhong1999)
with strict adherence to KISS and DRY principles:
1. **Reactive Handshake**: Replaces the legacy blocking `CountDownLatch`
with a reactive `Sinks.One<Void>` handshake.
2. **Unified Single-Owner Cleanup**:
- Eliminates redundant ownership concepts (`listenerUnregistered` flag
and `unregisterIdleOnce` helper).
- All cleanup actions (`selectedMailbox.unregisterIdle`, line handler
popping, and sink completion) are atomically coordinated by a single cleanup
owner governed by `idleActive.compareAndSet(true, false)`.
3. **Resilience to Transient Push Failures**:
- Errors encountered while pushing individual unsolicited event
notifications are safely logged in debug rather than tearing down the entire
IDLE session. The client remains in IDLE and can cleanly exit via `DONE`.
- Stripped redundant state and fields (`selectedMailbox`,
`lineHandlerState`) from `IdleMailboxListener`.
4. **Protocol Compliance & Empty Line Handling (RFC 3501)**:
- When an active session sends an empty continuation line instead of
`DONE`, the server replies with a tagged `BAD` response rather than silently
returning without responding to the IDLE tag. Early return without response is
strictly limited to disconnected sessions (`!session.isConnected()`).
- Fixed
`IMAPServerIdleTest.mailboxEventAfterDoneShouldNotPushUnsolicitedResponses`:
removed assertions prohibiting `EXISTS` on subsequent `NOOP` commands, honoring
RFC 3501 §6.1.2 which specifies that `NOOP` allows the server to send mailbox
status updates.
5. **Deterministic LineHandler Lifecycle**:
- Uses a dedicated `LineHandlerState` state machine (`NOT_INSTALLED`,
`INSTALLING`, `INSTALLED`, `REMOVAL_PENDING`, `REMOVED`) to guarantee
`popLineHandler()` is invoked exactly once, preventing double-pop on early
callbacks and correctly handling mid-push cancellations.
6. **Input Sanitization**:
- Sanitizes unrecognized continuation input against CRLF and log
injection when constructing tagged `BAD` responses.
---
### Modifications
- **`protocols/imap`**:
- `ImapSession`: Add default method `isConnected()` to query transport
connection state.
- `SelectedMailbox`: Add identity-aware
`unregisterIdle(ReactiveEventListener listener)`.
- `SelectedMailboxImpl`: Implement identity-aware unregistration via
`idleEventListener.compareAndSet(listener, null)`.
- `IdleProcessor`:
- Unified single-owner cleanup model (`idleActive.compareAndSet(true,
false)`).
- Removed `listenerUnregistered` parameter across all internal methods
(`idle`, `registerIdleListener`, `installLineHandler`, `createIdleLineHandler`,
`scheduleHeartbeat`, `cleanupIdle`).
- Fixed empty line continuation to reply `BAD` for active connections.
- Simplified `IdleMailboxListener`: logs push errors without aborting
IDLE; removed unused fields.
- Protocol token comparisons use `Locale.ROOT`.
- `IdleProcessorLifecycleTest`:
- Tests single `popLineHandler()` invocation on early callbacks and
mid-push cancellations.
- Verified single `unregisterIdle()` execution by the cleanup owner upon
registration and line-push failures.
- Removed unused `Mockito.times` import.
- `IdleProcessorSanitizationTest`: Unit tests for continuation input
sanitization.
- `SelectedMailboxImplTest`: Verifies stale unregister calls do not clear
newly registered listeners.
- **`server/protocols/protocols-imap4`**:
- `NettyImapSession`: Implement `isConnected()` bound to
`channel.isActive()`.
- `IMAPServerIdleTest`:
- Fixed assertion in
`mailboxEventAfterDoneShouldNotPushUnsolicitedResponses` to align with RFC 3501
§6.1.2.
- Integration tests for invalid continuation recovery, mid-IDLE
`LOGOUT`, and metric cleanup on client disconnect.
---
### Result
- Simplified code base (**-48 lines net reduction**, cleaner signatures, and
zero redundant abstractions).
- Full compliance with RFC 3501.
- Safe, non-blocking lifecycle and deterministic cleanup across all
execution paths.
--
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]
