Add HTA-side implementation of sync-interrupted scenario (HT-508). Also replace deprecated AsyncTask.#13
Conversation
…stead of hardcoding "success". Replace deprecated AsyncTask. Added handling for the HttpRequest from HT which now can have either "sync_success" or "sync_interrupted". Formerly the user would see a success message in either case. Now, however, when a sync is cancelled the user will see a message indicating that sync did not complete. Replaced AsyncTask (which is deprecated) with Executors and Handlers. This still contains debug output from the investigation activities. A future commit (very soon, hopefully) will remove this in PR preparation.
…Also replace deprecated AsyncTask code. Formerly HT sent HTA "sync_success" regardless of whether 'mergeCompleted' was True. Now the False case leads to HTA receiving "sync_interrupted" and HTA informs the user that sync did not complete successfully. Also, deprecated class 'AsyncTask' is replaced with 'Executors' and 'Handlers'.
tombogle
left a comment
There was a problem hiding this comment.
@tombogle reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @wmergenthal)
app/src/main/java/org/sil/hearthis/AcceptNotificationHandler.java line 43 at r1 (raw file):
// // NOTE: like several things in HearThisAndroid, HttpRequest is deprecated. It will be // replaced with something more appropriate, hopefully soon.
Options:
-
Use
HttpURLConnection(built-in JDK/Android)
→ Standard replacement for most simple request/response work.-
Pros: No external dependency.
-
Cons: Verbose API; no built-in high-level features.
-
-
Use
OkHttp(square.github.io/okhttp)
→ De facto standard for Android networking.-
Pros: Clean API, better performance, supports HTTP/2, widely adopted.
-
Cons: Adds dependency, but well worth it in most apps.
-
app/src/main/java/org/sil/hearthis/AcceptNotificationHandler.java line 49 at r1 (raw file):
String s2 = s1.substring(s1.indexOf(' ') + 1, s1.lastIndexOf(' ')); String status = s2.substring(s2.indexOf('=') + 1);
This logic could be done in a less cryptic way:
String s1 = request.getRequestLine().getUri(); // avoid toString() if possible
URI uri = new URI(s1);
String query = uri.getQuery(); // e.g., "status=123&foo=bar"
String status = null;
for (String param : query.split("&")) {
String[] pair = param.split("=");
if (pair.length == 2 && pair[0].equals("status")) {
status = pair[1];
break;
}
}
Alternatively, it could be done very simply with a regular expression. But using URI is probably safer and makes the intent clear.
app/src/main/java/org/sil/hearthis/SyncActivity.java line 166 at r1 (raw file):
socket.send(packet); } catch (UnknownHostException e) { e.printStackTrace();
In the case of an exception, is there anything else we need to do to give feedback to the user and/or prevent the HTA app from getting stuck in a bad state?
app/src/main/java/org/sil/hearthis/SyncActivity.java line 280 at r1 (raw file):
setProgress(getString(R.string.sync_interrupted)); } else { // Should never happen. Not sure what to do here, if anything...
For the sake of future proofing, we should probably handle any unexpected message by letting the use know that they are probably using a version of HTA that is not fully compatible with HT desktop. Then probably we should treat it similar to the "interrupted" case, since we probably can't safely assume it was completed successfully.
Instead of raw string objects, use a URI object for status extraction from the HttpRequest from HT. Resulting logic is longer but clearer.
Treat similarly to when sync is interrupted. Show the user a non-success message and suggest that they verify that HT/HTA versions are close enough that HTA knows about all possible HT sync statuses.
|
|
||
| if (status == null) { | ||
| // Something went wrong. Make sure the user sees a non-success message. | ||
| status = "sync_interrupted"; |
There was a problem hiding this comment.
I wonder if we should display a different message in the case where we get an unexpected status message. I assume the processing should be the same (since we can't possibly know how else to interpret the unexpected message), but if this ever happens, we could give the user some indication that we got an unexpected message and suggest they check for an update. We could even include in the query (from HT) an indication of the minimum version of HTA known to handle the particular status message. Then if we ever get an unexpected status and it includes a version number as well, we could tell the user the minimum version of HTA that is compatible with the version of HT they are using. The HTA code would then look something like this:
String status = null;
String minHtaVersion = null;
try {
String s1 = request.getRequestLine().getUri();
URI uri = new URI(s1);
String query = uri.getQuery();
if (query != null) {
for (String param : query.split("&")) {
String[] pair = param.split("=", 2); // limit=2 in case value contains '='
if (pair.length == 2) {
if (pair[0].equals("message")) {
status = pair[1];
} else if (pair[0].equals("minHtaVersion")) {
minHtaVersion = pair[1];
}
}
}
}
} catch (Exception e) {
e.printStackTrace();
}
if (status == null || isUnexpectedStatus(status)) {
if (minHtaVersion != null) {
showError("Unexpected sync status received. This version of HearThis for Android appears to be incompatible with your version of HearThis. Please update HearThis for Android to at least version " + minHtaVersion + ".");
} else {
showError("Unexpected sync status received. Please check for updates.");
}
status = "sync_interrupted"; // Or possibly a distinct status
}
Nominal case works. Still need to test watchdog when HT fails to complete its side of sync. Debug output still present (which helps greatly understanding the mechanism).
No substantive changes. Just captures debug stmts producing the output captured in 20251001_02_HTA_sync_watchdog_all_ok.log, where watchdog is seen to work correctly for both success and fail scenarios.
Watchdog was being started too early. If the Android user was not quick to scan the QR code after getting into that mode, the watchdog could have timed out before the sync even started. Now we don't start it until Android has sent its UDP packet to PC to initiate a sync. Also, multiple watchdog shutdown() calls are now consolidated into a single one. Add comments to the new Watchdog class. Remove some dead code and improve some debug output stmts.
Without the Continue button the user can't get back to the home screen unless a subsequent sync retry is successful. To make the Continue button usable, add call to AcceptNotificationHandler like all the other spots have.
|
From an error handling/reporting standpoint, if we did get a query, it might be a good idea to display and/or log it in a way that it could be reported to us so we could try to figure out what happened. |
tombogle
left a comment
There was a problem hiding this comment.
@tombogle reviewed 2 files and all commit messages.
Reviewable status: 2 of 7 files reviewed, 6 unresolved discussions (waiting on @wmergenthal).
This is a coordinating change with the HT side. Last fall we had decided to change a few wordings, one of which is to change "sync_interrupted" to "sync_canceled". This comm does that as well as commenting out several debug output stmts. Debug output will be removed altogether before the next push, but this is an intermediate snapshot commit.
There are a handful of debug output things I want to capture in a way that enables easy and reliable transmission to our tech support. These are commented out along with a notation to "implement for tech support". (1) Timeout reduction, (2) minHtaVersion and (3) exception handling fixes are being postponed until after the Code-a-thon. Some of these might get done *during* the Code-a-thon, but even if they don't it will be preferable to develop these 3 things on the updated code base. The intent of this commit is to complete all needed changes requested for Pull Request sillsdev#13.
|
I hope this is helpful and doesn't just add more noise, but I now trust devin review more than a human to do reviews. https://app.devin.ai/review/sillsdev/HearThisAndroid/pull/13#issuecomment-3910931106
|
| public Watchdog(long timeout, TimeUnit unit, Runnable onTimeout) { | ||
| this.timeout = timeout; | ||
| this.unit = unit; | ||
| this.onTimeout = onTimeout; | ||
| } |
There was a problem hiding this comment.
🔴 Watchdog timer never starts its initial countdown after construction
The Watchdog is constructed at SyncActivity.java:178 but pet() is never called to kick off the initial timer countdown. The Watchdog constructor only stores parameters — it does not schedule anything. The pet() method is the only way to start the timer, and it is only called from receivingFile() (line 342) and sendingFile() (line 354).
Root Cause and Impact
If the sync operation stalls before any files are transferred (e.g., the PC receives the UDP packet but fails before sending/requesting any files), neither receivingFile() nor sendingFile() will ever be called, so pet() is never invoked, the watchdog timer never starts, and the timeout callback never fires. The app will be stuck indefinitely with no error message and no way for the user to proceed (the Continue button is never enabled).
The Watchdog's own documentation says "Once instantiated and started, it counts down from its timeout value" but the constructor doesn't start anything. A pet() call is needed right after construction at line 184 to start the initial countdown.
Prompt for agents
In app/src/main/java/org/sil/hearthis/SyncActivity.java, after the Watchdog is constructed at line 178-184, add a call to watchdog.pet() to start the initial countdown timer. This should be placed right after the Watchdog constructor call (around line 184, after the closing parenthesis and semicolon of the Watchdog construction). Alternatively, modify the Watchdog constructor in app/src/main/java/org/sil/hearthis/Watchdog.java at lines 25-29 to automatically schedule the first timeout by calling pet() at the end of the constructor.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // is unable to complete a sync operation. Getting here means we got a notification | ||
| // from the PC. It should contain the final sync status, but even if it doesn't, the | ||
| // sync operation *is* complete and the watchdog should be turned off. | ||
| watchdog.shutdown(); |
There was a problem hiding this comment.
🔴 NullPointerException on watchdog.shutdown() when watchdog has not been initialized
At SyncActivity.java:299, watchdog.shutdown() is called unconditionally in onNotification(), but watchdog may be null if the notification arrives before the QR code scanning and executor task assigns the watchdog field.
Root Cause and Impact
The watchdog field is only assigned inside a background executor task at line 178, which runs after a QR code is scanned. However, onNotification() can be called in several scenarios where watchdog is still null:
- If the
AcceptNotificationHandlerreceives a notification with astatusparameter from the PC before the user has scanned a QR code (e.g., from a stale/leftover request). - If the old HearThis PC (without the new
minHtaVersion/statusprotocol) sends a plain/notifyrequest —AcceptNotificationHandlerwill setstatus = "sync_error"and callonNotification("sync_error"), which reachesSyncActivity.onNotification()whilewatchdogis stillnull.
In any of these cases, watchdog.shutdown() at line 299 will throw a NullPointerException, crashing the app.
| watchdog.shutdown(); | |
| if (watchdog != null) { | |
| watchdog.shutdown(); | |
| } | |
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| @Override | ||
| public void receivingFile(final String name) { | ||
| watchdog.pet(); |
There was a problem hiding this comment.
🔴 NullPointerException on watchdog.pet() when watchdog has not been initialized
At SyncActivity.java:342 and SyncActivity.java:354, watchdog.pet() is called unconditionally in receivingFile() and sendingFile(). However, watchdog can be null if file transfer callbacks arrive before the watchdog is created.
Root Cause and Impact
The watchdog field is assigned inside a background executor task at line 178. The receivingFile() and sendingFile() callbacks are triggered by the HTTP server handlers (AcceptFileHandler and RequestFileHandler) which use a static listener field set in onResume() at lines 91-92. Since the listener is registered in onResume() (which runs before any QR code scan), it's possible for file transfer requests to arrive and trigger these callbacks before the watchdog is initialized — for example, if the PC initiates file transfers from a previous session or if there's a race between the executor creating the watchdog and the server receiving file requests.
Calling pet() on a null watchdog will throw a NullPointerException, crashing the app.
| watchdog.pet(); | |
| if (watchdog != null) { | |
| watchdog.pet(); | |
| } | |
Was this helpful? React with 👍 or 👎 to provide feedback.
| } else if (pair[0].equals("minHtaVersion")) { | ||
| minHtaVersion = pair[1]; | ||
| return; |
There was a problem hiding this comment.
🟡 AcceptNotificationHandler early return for minHtaVersion sends no HTTP response
At AcceptNotificationHandler.java:70, when the handler receives a minHtaVersion query parameter, it does return without setting any response entity on the HttpResponse object.
Root Cause and Impact
When HearThis PC sends POST /notify?minHtaVersion=1.0 HTTP/1.1, the handler stores the version and returns immediately at line 70 without calling response.setEntity(...). The other code paths (lines 85-88) always set a response entity. The HTTP server (HttpService.handleRequest) will send back a response with no body, which may cause the HearThis PC client to fail or behave unexpectedly if it expects a response body. At minimum, a response entity should be set before returning to maintain the API contract.
| } else if (pair[0].equals("minHtaVersion")) { | |
| minHtaVersion = pair[1]; | |
| return; | |
| } else if (pair[0].equals("minHtaVersion")) { | |
| minHtaVersion = pair[1]; | |
| response.setEntity(new StringEntity("ok")); | |
| return; | |
Was this helpful? React with 👍 or 👎 to provide feedback.

Formerly HT sent HTA "sync_success" regardless of whether 'mergeCompleted' was True. Now the False case leads to HTA receiving sync status "sync_interrupted". HTA parses the HttpRequest from HT to extract the status, then informs the user. One new string is added to HTA for the interrupt-or-cancelled case.
Also, deprecated class 'AsyncTask' is replaced with 'Executors' and 'Handlers'.
This change is