Fix reconnection hang on HTTP proxy connect failures and stalled handshakes#1275
Fix reconnection hang on HTTP proxy connect failures and stalled handshakes#1275rishabhagl wants to merge 3 commits into
Conversation
…dshakes - Handle exceptional ConnectFuture completion in pollConnectFuture() instead of treating it as still-pending, which previously left connectFuture non-null forever and blocked re-connection. - Add a connectTimeoutMillis watchdog to cancel and reset pending connects that never resolve (no session, no exception) — common with stalled HTTP CONNECT proxy handshakes. - Recreate the ProxyConnector via setupIoConnector() after cancelling a stuck pending attempt, and reset lastConnectTime/lastReconnectAttemptTime to avoid an immediate re-fire on the next reconnect tick. - Add unit tests covering the changes.
`pollConnect()` is only called from methods which originate from `run()` which is called by a single thread from the executor, so `synchronized` could be removed.
| } | ||
|
|
||
| try { | ||
| setupIoConnector(); |
There was a problem hiding this comment.
Why is this only done on a ProxyConnector? I assume because it has some internal state that needs to be handled differently? Maybe add some explanation?
Edit: I'm still thinking about if this could conflict with the dispose-logic.
There was a problem hiding this comment.
I got this exception when reused IoConnector for proxy.
java.lang.IllegalStateException: handler cannot be set while the service is active.
at org.apache.mina.core.service.AbstractIoService.setHandler(...)
at org.apache.mina.proxy.ProxyConnector.connect0(...)
This exception does not occure when reusing the IoConnector in non-proxy connection timeout, where the server does not reply TCP SYN.
by explanation, you mean adding code comment?
|
This is a tough one. The refactoring looks solid to me and I like it. It seems like this specific scenario was investigated thoroughly, but it is hard for me to prove or disprove the validity of this change. Would you be able to demo the failing scenario without the fix with some end 2 end unit tests? It doesn't need to happen every time as these failures tend to be time dependent, but it would be cool to see what is actually failing? There is some code aiding in HTTP proxy auth Also what HTTP proxy software and version you are using? |
3 participants
Summary
Fixes #1274 — reconnection permanently stalls when connecting through an HTTP proxy if the proxy connect fails with an exception, or if the CONNECT handshake stalls without resolving the ConnectFuture at all.
Problem
IoSessionInitiator.pollConnectFuture()only checkedfuture.getSession() != null. If the future instead completed exceptionally (e.g. proxy returns 407, or the CONNECT is rejected), the code fell through into the "still pending" branch, which just logged a message and never clearedconnectFutureor calledhandleConnectException(). SinceshouldReconnect()requiresconnectFuture == null, the initiator got stuck and never retried.Separately, a stalled proxy handshake (no session, no exception — MINA's proxy filter wedged mid-negotiation) had no timeout at all and could hang indefinitely.
Fix
pollConnectFuture()now explicitly checksfuture.getException()and routes it throughhandleConnectException(), clearingconnectFutureimmediately.connectTimeoutMillis-based watchdog: if a connect attempt has been pending longer than this,cancelAndResetPendingConnectAttempt()is invoked to:IoSessionand cancel the future,ioConnector instanceof ProxyConnector, cancel the proxy connector's internal connect future and recreate the connector viasetupIoConnector(),lastConnectTime/lastReconnectAttemptTimeto the current time so the next scheduled reconnect doesn't fire immediately (previouslypendingMilliswould already exceedReconnectInterval, causing a premature/rapid retry loop) or never (if left unset).Testing
Added
IoSessionInitiatorConnectTaskTest, which targets the privateIoSessionInitiator.ConnectTaskpolling state machine via reflection (the fix lives inside this private class, so tests constructConnectTaskdirectly and invokepollConnectFuture(),cancelAndResetPendingConnectAttempt(), andhandleConnectException()reflectively).Coverage includes:
connectTimeoutMillis— future is cancelled, timeout events are logged,onConnectExceptionfires,connectFutureclears, failure count increments once, and both timers reset soshouldReconnect()correctly waits out the fullReconnectInterval.lastConnectTimeupdated.TestProxyConnectorstub) — confirms the pending future is cancelled,cancelConnectFuture()is called, and the connector instance is recreated (not reused).cancelAndResetPendingConnectAttempt— closes a half-open session before cancelling the future and separately confirms proxy-connector cancellation/recreation in isolation.handleConnectException— unwraps nested causes, increments failure count, clears the future, and notifies the state listener.All existing tests in
quickfixj-corepass locally with this change.Related