device: Wait device to finish for a timeout before completing VerifyStop

When a device has reported the verification status the client should
call VerifyStop to stop the device, however this under the hood may lead
to a premature cancellation, causing the device not to react as expected
in case the finger is still on the sensor or in case it may return to us
some errors that we may want to handle (like the data-missing one).

So, in case we are about to stop the verification and the operation is
still in process, wait for a maximum timeout before proceed to the
cancellation.

However, while waiting, the action may be also cancelled because of a
call to Release() or because the client vanished, and in such case we
have to ensure that the current invocation is saved for being invoked by
stoppable_action_completed() when callback will return. That will also
unset it, and that's a clear indication for us that it has been already
consumed, and thus that we can just return doing nothing else.

Fixes: #100

src/device.c

@@ -33,6 +33,8 @@
 #include "fprintd.h"
 #include "storage.h"
 
+#define VERIFY_STOP_DEVICE_WAIT 1 /* Seconds to wait for the device to complete */
+
 static const char *FINGERS_NAMES[] = {
   [FP_FINGER_UNKNOWN]      = "unknown",
   [FP_FINGER_LEFT_THUMB]   = "left-thumb",
@@ -94,6 +96,8 @@ typedef struct
   FpDevice        *dev;
   SessionData     *_session;
 
+  guint            verify_stop_wait_timeout_id;
+
   PolkitAuthority *auth;
 
   /* Hashtable of connected clients */
@@ -235,6 +239,7 @@ fprint_device_finalize (GObject *object)
   FprintDevice *self = (FprintDevice *) object;
   FprintDevicePrivate *priv = fprint_device_get_instance_private (self);
 
+  g_clear_handle_id (&priv->verify_stop_wait_timeout_id, g_source_remove);
   g_hash_table_destroy (priv->clients);
   session_data_set_new (priv, NULL, NULL);
   g_clear_object (&priv->auth);
@@ -1486,11 +1491,43 @@ fprint_device_verify_start (FprintDBusDevice      *dbus_dev,
   return TRUE;
 }
 
+static gboolean
+verify_stop_wait_timeout (gpointer data)
+{
+  guint *timeout_id = data;
+
+  *timeout_id = 0;
+  return FALSE;
+}
+
+static gboolean
+verify_has_completed (FprintDevice *rdev)
+{
+  FprintDevicePrivate *priv = fprint_device_get_instance_private (rdev);
+
+  if (!priv->current_cancellable ||
+      g_cancellable_is_cancelled (priv->current_cancellable))
+    return TRUE;
+
+  switch (priv->current_action)
+    {
+    case ACTION_VERIFY:
+      return !priv->verify_data;
+
+    case ACTION_IDENTIFY:
+      return !priv->identify_data;
+
+    default:
+      g_assert_not_reached ();
+    }
+}
+
 static gboolean
 fprint_device_verify_stop (FprintDBusDevice      *dbus_dev,
                            GDBusMethodInvocation *invocation)
 {
   FprintDevice *rdev = FPRINT_DEVICE (dbus_dev);
+  FprintDevicePrivate *priv = fprint_device_get_instance_private (rdev);
 
   g_autoptr(GError) error = NULL;
 
@@ -1506,6 +1543,40 @@ fprint_device_verify_stop (FprintDBusDevice      *dbus_dev,
       return TRUE;
     }
 
+  if (!verify_has_completed (rdev))
+    {
+      g_autoptr(SessionData) session = session_data_get (priv);
+
+      if (session->verify_status_reported)
+        {
+          /* If we got a status report we need to delay the cancellation
+           * of the action, leaving the device some more time to complete
+           * the operation (and in case return the real error) before proceed
+           * in cancelling it.
+           * In case Release or client vanished while waiting the invocation
+           * will be handled by stoppable_action_completed() during cancellation
+           */
+          g_assert (priv->verify_stop_wait_timeout_id == 0);
+
+          priv->verify_stop_wait_timeout_id =
+            g_timeout_add_seconds (VERIFY_STOP_DEVICE_WAIT, verify_stop_wait_timeout,
+                                   &priv->verify_stop_wait_timeout_id);
+
+          g_assert (priv->current_cancel_invocation == NULL);
+          priv->current_cancel_invocation = invocation;
+
+          while (priv->verify_stop_wait_timeout_id && !verify_has_completed (rdev))
+            g_main_context_iteration (NULL, TRUE);
+
+          g_clear_handle_id (&priv->verify_stop_wait_timeout_id, g_source_remove);
+
+          if (!priv->current_cancel_invocation)
+            return TRUE;
+
+          priv->current_cancel_invocation = NULL;
+        }
+    }
+
   stoppable_action_stop (rdev, invocation);
 
   return TRUE;

tests/fprintd.py

@@ -387,14 +387,17 @@ class FPrintdTest(dbusmock.DBusTestCase):
         with Connection(self.sockaddr) as con:
             self.send_image(image, con)
 
-    def send_finger_automatic(self, automatic, con=None):
+    def send_finger_automatic(self, automatic, con=None, iterate=True):
         # Set whether finger on/off is reported around images
         if con:
             con.sendall(struct.pack('ii', -3, 1 if automatic else 0))
             return
 
         with Connection(self.sockaddr) as con:
-            self.send_finger_automatic(automatic, con=con)
+            self.send_finger_automatic(automatic, con=con, iterate=iterate)
+
+        while iterate and ctx.pending():
+            ctx.iteration(False)
 
     def send_finger_report(self, has_finger, con=None, iterate=True):
         # Send finger on/off
@@ -1944,6 +1947,7 @@ class FPrintdVirtualDeviceVerificationTests(FPrintdVirtualDeviceBaseTest):
         cls.enroll_finger = 'left-middle-finger'
         cls.verify_finger = cls.enroll_finger
         cls.stop_on_teardown = True
+        cls.releases_on_teardown = True
 
     def setUp(self):
         super().setUp()
@@ -1954,6 +1958,7 @@ class FPrintdVirtualDeviceVerificationTests(FPrintdVirtualDeviceBaseTest):
     def tearDown(self):
         if self.stop_on_teardown:
             self.device.VerifyStop()
+        if self.releases_on_teardown:
             self.device.Release()
         super().tearDown()
 
@@ -2097,6 +2102,60 @@ class FPrintdVirtualDeviceVerificationTests(FPrintdVirtualDeviceBaseTest):
             self.send_error(con=con)
             self.wait_for_result(max_wait=200, expected='verify-match')
 
+    def test_verify_stop_restarts_immediately(self):
+        self.send_image('tented_arch')
+        self.wait_for_result()
+        self.assertTrue(self._verify_stopped)
+        self.assertEqual(self._last_result, 'verify-no-match')
+
+        self.call_device_method_async('VerifyStop', '()', [])
+        self.call_device_method_async('VerifyStart', '(s)', [self.verify_finger])
+
+        self.wait_for_device_reply(expected_replies=2)
+
+    def test_verify_stop_waits_for_completion(self):
+        self.stop_on_teardown = False
+
+        with Connection(self.sockaddr) as con:
+            self.send_finger_automatic(False, con=con)
+            self.send_finger_report(True, con=con)
+            self.send_image('tented_arch', con=con)
+            self.wait_for_result()
+
+        self.assertTrue(self._verify_stopped)
+        self.assertEqual(self._last_result, 'verify-no-match')
+
+        self.call_device_method_async('VerifyStop', '()', [])
+
+        def restart_verify(abort=False):
+            self.call_device_method_async('VerifyStart', '(s)', [self.verify_finger])
+            with self.assertFprintError('AlreadyInUse'):
+                self.wait_for_device_reply(method='VerifyStart')
+
+            self.assertFalse(self.get_async_replies(method='VerifyStop'))
+            self._abort = abort
+
+        restart_verify()
+        GLib.timeout_add(100, restart_verify)
+        GLib.timeout_add(300, restart_verify, True)
+        self.wait_for_result()
+
+    def test_verify_stop_waits_for_completion_waiting_timeout(self):
+        self.test_verify_stop_waits_for_completion()
+        self.wait_for_device_reply(method='VerifyStop')
+        self.assertTrue(self.get_async_replies(method='VerifyStop'))
+
+    def test_verify_stop_waits_for_completion_is_stopped_by_release(self):
+        # During the release here we're testing the case in which
+        # while we're waiting for VerifyStop to return, Release stops the
+        # verification, making the invocation to return
+        self.releases_on_teardown = False
+        self.test_verify_stop_waits_for_completion()
+        self.assertFalse(self.get_async_replies(method='VerifyStop'))
+        self.call_device_method_async('Release', '()', [])
+        self.wait_for_device_reply(method='Release')
+        self.assertTrue(self.get_async_replies(method='VerifyStop'))
+
 
 class FPrintdVirtualDeviceIdentificationTests(FPrintdVirtualDeviceVerificationTests):
     '''This class will just repeat the tests of FPrintdVirtualDeviceVerificationTests