-
-
Notifications
You must be signed in to change notification settings - Fork 5
Add HTA-side implementation of sync-interrupted scenario (HT-508). Also replace deprecated AsyncTask. #13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
5f52643
fd35b08
3727d83
399e532
678cd7c
00f8cb0
a875cf3
fd78761
f743ece
fc89607
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,15 +1,19 @@ | ||||||||||||
| package org.sil.hearthis; | ||||||||||||
|
|
||||||||||||
| import static org.sil.hearthis.AcceptNotificationHandler.notificationListeners; | ||||||||||||
|
|
||||||||||||
| import android.Manifest; | ||||||||||||
| import android.annotation.SuppressLint; | ||||||||||||
| import android.content.Intent; | ||||||||||||
| import android.content.pm.PackageManager; | ||||||||||||
| import android.os.AsyncTask; | ||||||||||||
| import android.os.Bundle; | ||||||||||||
|
|
||||||||||||
| import androidx.appcompat.app.AppCompatActivity; | ||||||||||||
| import androidx.core.app.ActivityCompat; | ||||||||||||
|
|
||||||||||||
| import android.os.Handler; | ||||||||||||
| import android.os.Looper; | ||||||||||||
| import android.util.Log; | ||||||||||||
| import android.util.SparseArray; | ||||||||||||
| import android.view.Menu; | ||||||||||||
| import android.view.MenuItem; | ||||||||||||
|
|
@@ -23,15 +27,20 @@ | |||||||||||
| import com.google.android.gms.vision.barcode.Barcode; | ||||||||||||
| import com.google.android.gms.vision.barcode.BarcodeDetector; | ||||||||||||
|
|
||||||||||||
| //import org.apache.http.entity.StringEntity; | ||||||||||||
|
|
||||||||||||
| import java.io.IOException; | ||||||||||||
| import java.net.DatagramPacket; | ||||||||||||
| import java.net.DatagramSocket; | ||||||||||||
| import java.net.InetAddress; | ||||||||||||
| import java.net.NetworkInterface; | ||||||||||||
| import java.net.SocketException; | ||||||||||||
| import java.net.UnknownHostException; | ||||||||||||
| //import java.net.UnknownHostException; | ||||||||||||
| import java.util.Date; | ||||||||||||
| import java.util.Enumeration; | ||||||||||||
| import java.util.concurrent.Executors; | ||||||||||||
| import java.util.concurrent.ExecutorService; | ||||||||||||
| import java.util.concurrent.TimeUnit; | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| public class SyncActivity extends AppCompatActivity implements AcceptNotificationHandler.NotificationListener, | ||||||||||||
|
|
@@ -44,11 +53,12 @@ public class SyncActivity extends AppCompatActivity implements AcceptNotificatio | |||||||||||
| SurfaceView preview; | ||||||||||||
| int desktopPort = 11007; // port on which the desktop is listening for our IP address. | ||||||||||||
| private static final int REQUEST_CAMERA_PERMISSION = 201; | ||||||||||||
| private static final int WATCHDOG_TIMEOUT_SECONDS = 10; // match the HearThis timeout? | ||||||||||||
| boolean scanning = false; | ||||||||||||
| TextView progressView; | ||||||||||||
|
|
||||||||||||
| private BarcodeDetector barcodeDetector; | ||||||||||||
| private CameraSource cameraSource; | ||||||||||||
| private Watchdog watchdog; | ||||||||||||
|
|
||||||||||||
| @Override | ||||||||||||
| protected void onCreate(Bundle savedInstanceState) { | ||||||||||||
|
|
@@ -125,6 +135,8 @@ public void release() { | |||||||||||
| // Toast.makeText(getApplicationContext(), "To prevent memory leaks barcode scanner has been stopped", Toast.LENGTH_SHORT).show(); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| // Replacing 'AsyncTask' (deprecated) with 'Executors' and 'Handlers' in this method is inspired by: | ||||||||||||
| // https://stackoverflow.com/questions/58767733/the-asynctask-api-is-deprecated-in-android-11-what-are-the-alternatives | ||||||||||||
| @Override | ||||||||||||
| public void receiveDetections(Detector.Detections<Barcode> detections) { | ||||||||||||
| final SparseArray<Barcode> barcodes = detections.getDetectedItems(); | ||||||||||||
|
|
@@ -145,15 +157,49 @@ public void run() { | |||||||||||
| // provide some users a clue that all is not well. | ||||||||||||
| ipView.setText(contents); | ||||||||||||
| preview.setVisibility(View.INVISIBLE); | ||||||||||||
| SendMessage sendMessageTask = new SendMessage(); | ||||||||||||
| sendMessageTask.ourIpAddress = getOurIpAddress(); | ||||||||||||
| sendMessageTask.execute(); | ||||||||||||
| ExecutorService executor = Executors.newSingleThreadExecutor(); | ||||||||||||
| Handler handler = new Handler(Looper.getMainLooper()); | ||||||||||||
| executor.execute(() -> { | ||||||||||||
| // Background work: send UDP packet to IP address given in the QR code. | ||||||||||||
| try { | ||||||||||||
| String ourIpAddress = getOurIpAddress(); | ||||||||||||
| //Log.d("Sync", "SyncActivity.run, ourIpAddress = " + ourIpAddress); // implement for tech support | ||||||||||||
| String ipAddress = ipView.getText().toString(); | ||||||||||||
| InetAddress receiverAddress = InetAddress.getByName(ipAddress); | ||||||||||||
| DatagramSocket socket = new DatagramSocket(); | ||||||||||||
| byte[] ipBytes = ourIpAddress.getBytes("UTF-8"); | ||||||||||||
| DatagramPacket packet = new DatagramPacket(ipBytes, ipBytes.length, receiverAddress, desktopPort); | ||||||||||||
| socket.send(packet); | ||||||||||||
|
|
||||||||||||
| // Don't create and start the watchdog until we KNOW that we are doing a sync. | ||||||||||||
| // At this point we have responded to the PC's sync offer and are indeed committed. | ||||||||||||
| // NOTE: inside the braces is the 'onTimeout' mitigation code, running only if | ||||||||||||
| // timeout occurs. | ||||||||||||
| watchdog = new Watchdog(WATCHDOG_TIMEOUT_SECONDS, TimeUnit.SECONDS, () -> { | ||||||||||||
| //Log.d("Sync", "Watchdog, TIMED OUT, setting Error"); // implement for tech support | ||||||||||||
| for (AcceptNotificationHandler.NotificationListener listener: notificationListeners.toArray(new AcceptNotificationHandler.NotificationListener[notificationListeners.size()])) { | ||||||||||||
| listener.onNotification("sync_error"); | ||||||||||||
| } | ||||||||||||
| setProgress(getString(R.string.sync_error)); | ||||||||||||
| }); | ||||||||||||
| //Log.d("Sync", "SyncActivity.run, watchdog started, timeout = " + WATCHDOG_TIMEOUT_SECONDS + " secs"); // implement for tech support | ||||||||||||
| } catch (IOException ioe) { | ||||||||||||
| // Note: this also catches UnknownHostException, a subclass of IOException | ||||||||||||
| for (AcceptNotificationHandler.NotificationListener listener : notificationListeners.toArray(new AcceptNotificationHandler.NotificationListener[notificationListeners.size()])) { | ||||||||||||
| listener.onNotification("sync_canceled"); | ||||||||||||
| } | ||||||||||||
| //Log.d("Sync", "SyncActivity.run, got exception: " + ioe); // implement for tech support | ||||||||||||
| ioe.printStackTrace(); | ||||||||||||
| } | ||||||||||||
| handler.post(() -> { | ||||||||||||
| // Background work done, no associated foreground work needed. | ||||||||||||
| }); | ||||||||||||
| }); | ||||||||||||
| cameraSource.stop(); | ||||||||||||
| cameraSource.release(); | ||||||||||||
| cameraSource = null; | ||||||||||||
| } | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| } | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
@@ -216,11 +262,8 @@ private String getOurIpAddress() { | |||||||||||
| if (inetAddress.isSiteLocalAddress()) { | ||||||||||||
| return inetAddress.getHostAddress(); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| } catch (SocketException e) { | ||||||||||||
| // TODO Auto-generated catch block | ||||||||||||
| e.printStackTrace(); | ||||||||||||
|
|
@@ -248,7 +291,33 @@ public boolean onOptionsItemSelected(MenuItem item) { | |||||||||||
| @Override | ||||||||||||
| public void onNotification(String message) { | ||||||||||||
| AcceptNotificationHandler.removeNotificationListener(this); | ||||||||||||
| setProgress(getString(R.string.sync_success)); | ||||||||||||
|
|
||||||||||||
| // The watchdog timer prevents the Android app from getting stuck if the PC side | ||||||||||||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 NullPointerException on watchdog.shutdown() when watchdog has not been initialized At Root Cause and ImpactThe
In any of these cases,
Suggested change
Was this helpful? React with 👍 or 👎 to provide feedback. |
||||||||||||
|
|
||||||||||||
| // HT-508: HearThis PC now includes sync status in its notification to the app. | ||||||||||||
| // We can now inform the user about whether sync succeeded. | ||||||||||||
| switch (message) { | ||||||||||||
| case "sync_success": | ||||||||||||
| setProgress(getString(R.string.sync_success)); | ||||||||||||
| break; | ||||||||||||
| case "sync_canceled": | ||||||||||||
| // Sync was canceled. | ||||||||||||
| setProgress(getString(R.string.sync_canceled)); | ||||||||||||
| break; | ||||||||||||
| case "sync_error": | ||||||||||||
| // Internal HTA error or incompatible versions of HT and HTA. | ||||||||||||
| setProgress(getString(R.string.sync_error)); | ||||||||||||
| break; | ||||||||||||
| default: | ||||||||||||
| // Not a sync status; should never happen. Raise an error. | ||||||||||||
| setProgress(getString(R.string.sync_error)); | ||||||||||||
| //Log.d("Sync", "onNotification.default, bad status: " + message); // implement for tech support | ||||||||||||
| break; | ||||||||||||
| } | ||||||||||||
| runOnUiThread(new Runnable() { | ||||||||||||
| @Override | ||||||||||||
| public void run() { | ||||||||||||
|
|
@@ -270,6 +339,8 @@ public void run() { | |||||||||||
|
|
||||||||||||
| @Override | ||||||||||||
| public void receivingFile(final String name) { | ||||||||||||
| watchdog.pet(); | ||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 NullPointerException on watchdog.pet() when watchdog has not been initialized At Root Cause and ImpactThe Calling
Suggested change
Was this helpful? React with 👍 or 👎 to provide feedback. |
||||||||||||
|
|
||||||||||||
| // To prevent excess flicker and wasting compute time on progress reports, | ||||||||||||
| // only change once per second. | ||||||||||||
| if (new Date().getTime() - lastProgress.getTime() < 1000) | ||||||||||||
|
|
@@ -280,32 +351,11 @@ public void receivingFile(final String name) { | |||||||||||
|
|
||||||||||||
| @Override | ||||||||||||
| public void sendingFile(final String name) { | ||||||||||||
| watchdog.pet(); | ||||||||||||
|
|
||||||||||||
| if (new Date().getTime() - lastProgress.getTime() < 1000) | ||||||||||||
| return; | ||||||||||||
| lastProgress = new Date(); | ||||||||||||
| setProgress("sending " + name); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| // This class is responsible to send one message packet to the IP address we | ||||||||||||
| // obtained from the desktop, containing the Android's own IP address. | ||||||||||||
| private class SendMessage extends AsyncTask<Void, Void, Void> { | ||||||||||||
|
|
||||||||||||
| public String ourIpAddress; | ||||||||||||
| @Override | ||||||||||||
| protected Void doInBackground(Void... params) { | ||||||||||||
| try { | ||||||||||||
| String ipAddress = ipView.getText().toString(); | ||||||||||||
| InetAddress receiverAddress = InetAddress.getByName(ipAddress); | ||||||||||||
| DatagramSocket socket = new DatagramSocket(); | ||||||||||||
| byte[] buffer = ourIpAddress.getBytes("UTF-8"); | ||||||||||||
| DatagramPacket packet = new DatagramPacket(buffer, buffer.length, receiverAddress, desktopPort); | ||||||||||||
| socket.send(packet); | ||||||||||||
| } catch (UnknownHostException e) { | ||||||||||||
| e.printStackTrace(); | ||||||||||||
| } catch (IOException e) { | ||||||||||||
| e.printStackTrace(); | ||||||||||||
| } | ||||||||||||
| return null; | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| package org.sil.hearthis; | ||
|
|
||
| import java.util.concurrent.*; | ||
| import android.util.Log; | ||
|
|
||
| /** | ||
| * This class implements a "watchdog" timer for the Android side of a HearThis sync operation. | ||
| * | ||
| * Once instantiated and started, it counts down from its timeout value (passed in). The timer | ||
| * is NOT supposed to get all the way down to 0. If it does, a problematic condition has arisen | ||
| * somewhere and the 'onTimeout' code runs in an effort to mitigate the problem. | ||
| * Calling pet() restarts a full countdown. The timeout value should be chosen such that it is | ||
| * longer than any normal interval between calls to pet(). Thus in a correctly working system, | ||
| * pet() keeps getting called well before the timer ever finishes counting down to 0 from its | ||
| * initial timeout value, and the 'onTimeout' code never runs. | ||
| */ | ||
|
|
||
| public class Watchdog { | ||
| private final ScheduledExecutorService scheduler = Executors.newSingleThreadScheduledExecutor(); | ||
| private ScheduledFuture<?> watchdogTask; | ||
| private final Runnable onTimeout; | ||
| private final long timeout; | ||
| private final TimeUnit unit; | ||
|
|
||
| public Watchdog(long timeout, TimeUnit unit, Runnable onTimeout) { | ||
| this.timeout = timeout; | ||
| this.unit = unit; | ||
| this.onTimeout = onTimeout; | ||
| } | ||
|
Comment on lines
+25
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Watchdog timer never starts its initial countdown after construction The Root Cause and ImpactIf 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 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 Prompt for agentsWas this helpful? React with 👍 or 👎 to provide feedback. |
||
|
|
||
| // Subsystems of interest call this method to restart the timer countdown. Basically this | ||
| // means: "At the moment all is well. We'll try to call again before your next deadline. If | ||
| // we don't, send for help." | ||
| public synchronized void pet() { | ||
| if (watchdogTask != null && !watchdogTask.isDone()) { | ||
| watchdogTask.cancel(false); | ||
| } | ||
| watchdogTask = scheduler.schedule(onTimeout, timeout, unit); | ||
| } | ||
|
|
||
| public void shutdown() { | ||
| scheduler.shutdownNow(); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 AcceptNotificationHandler early return for minHtaVersion sends no HTTP response
At
AcceptNotificationHandler.java:70, when the handler receives aminHtaVersionquery parameter, it doesreturnwithout setting any response entity on theHttpResponseobject.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 callingresponse.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.Was this helpful? React with 👍 or 👎 to provide feedback.