jon-valliere commented on code in PR #77:
URL: https://github.com/apache/mina/pull/77#discussion_r4239520587
##########
mina-core/src/main/java/org/apache/mina/filter/ssl/SSLHandlerG1.java:
##########
Review Comment:
+1 on the second option, with one change: please don't clear `mPendingError`.
AFAIK These errors are always fatal, because the engine is closed in both
directions, so there's nothing to recover by clearing. Today the clear never
actually runs: `receive_loop` rethrows `mPendingError` before reaching
`mPendingError = null`. So the error is effectively permanent, and other code
now relies on that:
- **G0 async tasks** (`ENABLE_ASYNC_TASKS = true` there): `execute_task`
calls `throw_pending_error` on the executor thread and swallows the exception.
That's only safe because the error stays stored and is rethrown on the next
`receive`/`ack`/`write`, on a filter-chain thread. With a real clear, the error
is consumed on the executor thread and the session never sees it. Rethrowing on
a filter-chain thread is the whole reason `throw_pending_error` exists.
- **Writes after the failure**: with the error cleared, `write_start` passes
(a fatal error doesn't set `mOutboundClosing`), `wrap` returns `CLOSED` with
nothing produced, and the request is quietly added to `mEncodeQueue`, so its
future never completes. With the error kept, the write fails right away.
So:
synchronized protected void throw_pending_error(NextFilter next) throws
SSLException {
SSLException sslException = mPendingError;
if (sslException != null) {
// Errors are fatal: the pending error stays set and is rethrown
on every subsequent call.
if (mEngine.isInboundDone() && mEngine.getHandshakeStatus() ==
HandshakeStatus.NEED_WRAP) {
write_handshake(next); // send the alert
}
throw sslException;
}
}
The throw-and-swallow in `execute_task` can then become a plain
`write_handshake(next)` to queue the alert, which says what it's for. Agree
with doing this in a follow-up for both G0 and G1, not in this PR.
--
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]
