I want to abort a connection attempt after a certain total timeout (as opposed to individual timeouts for each step of the connection), because it makes sense to have high timeouts for individual round-trips, e.g. 30s, but it doesn’t make sense to have an overall timeout of X*30s with X being the number of overall round-trips during connection setup.
I’m running XMPPTCPConnection.connect() and then .login() on a thread, and I want to re-use the connection after the failed connection attempt (as it might still hold XEP-0198 state).
However, both methods are synchronized, and the two potential methods to stop them (.disconnect() and .instantShutdown()) are synchronized as well, so they freeze until the connect/login has timed out.
Interrupting the Thread doesn’t yield the required result. Do I need to interrupt it multiple times?
How (else) can I abort the ongoing connection/login attempt in a timely fashion (let’s say within 100ms)?
P.S: I’m aware of XMPPTCPConnectionConfiguration.setConnectTimeout() but it will be awaited for each individual server in the SRV records, and it doesn’t take into account the DNS resolution nor the stream opening.
Further digging shows that making instantShutdown() non-synchronized will yield a partial effect.
https://stackoverflow.com/a/5395353/539443 says that you need to socket.close() to abort a connection attempt. However, the logic in connectInternal() will just consider this specific host/port as failed and continue with the next one, so we need some clean way to abort this. I’m pondering about adding a isShuttingDown member variable or checking for what the socket.isClosed() method returns.
Just a quick note: I think the better and cleaner approach could be to wrap the socket operations into a (Smack)Future, which is interruptable by cancelling it. A similar approach, wrapping the non-interruptable blocking operation into a thread, is also shown in the stackoverflow post you linked.
I don’t know whether cancelling the Future will also close the socket; from what SO says this won’t be the case, so the abandoned Future will eventually complete in the background?
I’m totally agnostic to how this problem is solved, though maybe spawning a new connecting-Thread from inside connectInternal() isn’t the most elegant approach.
It is an early draft and untested. While it has slightly more LoC than your PoC, it does not require the addition of an extra method to abort the connection attempt (abortConnect()). Users can simply interrupt the connecting thread, which I think is desirable and already hinted by the related methods signatures throwing InterruptedException.
Again this is untested, and I have to think more about if it is really sound. But until now, I wasn’t able to come up with an issue.
From a brief review (also untested) this looks like it will maybe abort the first connection attempt, but still try all the following addresses. This is why I introduced the signal variable and the InterruptedException.
Funnily my first version of the commit also changed catch (Exception e) to catch (IOException e) but I later reverted that in an attempt to reduce the diff noise. But of course this is required to that the InterruptedException is propagated upwards the call stack.