Adds retry mechanism to reconnect for more robustness #454
Conversation
| Thread.currentThread().interrupt(); | ||
| } | ||
|
|
||
| while ( ( retryLimit == 0 || retryCount <= retryLimit ) && bridge.isDisconnected() ){ |
There was a problem hiding this comment.
The spacing style isn't following the existing conventions. Can you reformat this patch to match what exists?
| @@ -439,6 +440,7 @@ private void finishConnection() { | |||
|
|
|||
| @Override | |||
| public void connect() { | |||
| connecting = true; | |||
There was a problem hiding this comment.
It might be easier to follow if you extract this out to another function like:
public void connect() {
connecting = true;
try {
connectInternal();
} finally {
connecting = false;
}
}
private void connectInternal() {
// previous contents of connect() ...
}
There was a problem hiding this comment.
This line didn't change with the addition of the intermediate function, but it is testing out ok on my build.
app/src/main/res/values/strings.xml
Outdated
| @@ -265,6 +265,11 @@ | |||
| <!-- Summary for the preference that forces the service to stay running in the background. --> | |||
| <string name="pref_conn_persist_summary">"Force connections to stay connected while in background"</string> | |||
|
|
|||
| <!-- Name for the retries size preference --> | |||
| <string name="pref_retries_title">"Persist retries"</string> | |||
| <!-- Description of the retries size preference --> | |||
There was a problem hiding this comment.
A bit more context of where this string appears will help. Translators only see these comments and not the context of what is retrying, where in the UI this will be seen, or what it is used for.
|
|
||
| bridge.startConnection(); | ||
|
|
||
| if ( retryCount < 8 ) { |
There was a problem hiding this comment.
Any kind of constants ("8" here and "2" below) should be extracted out to be in the class near the top.
| retryCount++; | ||
| Log.d(TAG, String.format("Retries %s session stat: %s", retryCount, bridge.isSessionOpen() )); | ||
|
|
||
| while(bridge.isConnecting()){ |
There was a problem hiding this comment.
Hmm, this could go on forever and drain batteries. There should be some kind of limit for how long it will wait.
| @@ -711,7 +720,43 @@ private void reconnectPending() { | |||
| if (bridge == null) { | |||
| continue; | |||
| } | |||
| bridge.startConnection(); | |||
|
|
|||
| int sleepVal = 500; | |||
There was a problem hiding this comment.
This should be extracted to a constant.
|
|
||
| while(bridge.isConnecting()){ | ||
| try { | ||
| Thread.sleep(500); |
There was a problem hiding this comment.
Looks like you forgot to re-use sleepVal here.
There was a problem hiding this comment.
Using minimum retry interval here because it remains constant and this is to wait on the SSH connect which should be more of a known value and not exceed ~30 sec. The sleepVal grows to address more general network issues which may not as quickly resolve.
Prototype of ARM semihosting support
|
|
…connectbot into i439-reconnect-after-net-loss
|
|
Not too fancy but implements a retry mechanism that falls back in frequency and eventually stops. Should be effective to reconnect in most cases. Fixes #439