fix locking for raising window

5 files changed, 151 insertions, 25 deletions

diff --git a/changes/bug_fix_raise_window_win b/changes/bug_fix_raise_window_win
new file mode 100644
index 00000000..ffad7c06
--- /dev/null
+++ b/changes/bug_fix_raise_window_win
@@ -0,0 +1,2 @@
+  o Fix incorrect handling of locks in windows so that stalled locks do not
+    avoid raising the first instance of the app. Closes: #2910
diff --git a/src/leap/gui/mainwindow.py b/src/leap/gui/mainwindow.py
index ba546fd0..6ee8b028 100644
--- a/src/leap/gui/mainwindow.py
+++ b/src/leap/gui/mainwindow.py
@@ -47,6 +47,7 @@ from leap.services.eip.providerbootstrapper import ProviderBootstrapper
 from leap.services.mail.smtpbootstrapper import SMTPBootstrapper
 from leap.platform_init import IS_WIN, IS_MAC
 from leap.platform_init.initializers import init_platform
+
 from leap.services.eip.vpnprocess import VPN
 
 from leap.services.eip.vpnlaunchers import (VPNLauncherException,
@@ -60,6 +61,7 @@ from leap.services.mail.smtpconfig import SMTPConfig
 
 if IS_WIN:
     from leap.platform_init.locks import WindowsLock
+    from leap.platform_init.locks import raise_window_ack
 
 from ui_mainwindow import Ui_MainWindow
 
@@ -1284,6 +1286,8 @@ class MainWindow(QtGui.QMainWindow):
         """
         Callback for the raise window event
         """
+        if IS_WIN:
+            raise_window_ack()
         self.raise_window.emit()
 
     def _do_raise_mainwindow(self):
@@ -1309,8 +1313,7 @@ class MainWindow(QtGui.QMainWindow):
         Triggered after aboutToQuit signal.
         """
         if IS_WIN:
-            lockfile = WindowsLock()
-            lockfile.release_lock()
+            WindowsLock.release_all_locks()
 
     def _cleanup_and_quit(self):
         """
diff --git a/src/leap/platform_init/locks.py b/src/leap/platform_init/locks.py
index c40c31d0..39b18648 100644
--- a/src/leap/platform_init/locks.py
+++ b/src/leap/platform_init/locks.py
@@ -28,12 +28,16 @@ from leap import platform_init
 
 if platform_init.IS_UNIX:
     from fcntl import flock, LOCK_EX, LOCK_NB
-else:
+else:  # WINDOWS
+    import datetime
     import glob
     import shutil
+    import time
 
     from tempfile import gettempdir
 
+    from leap.util import get_modification_ts, update_modification_ts
+
 logger = logging.getLogger(__name__)
 
 if platform_init.IS_UNIX:
@@ -144,6 +148,38 @@ if platform_init.IS_UNIX:
 
 if platform_init.IS_WIN:
 
+    # Time to wait (in secs) before assuming a raise window signal has not been
+    # ack-ed.
+
+    RAISE_WINDOW_TIMEOUT = 2
+
+    # How many steps to do while checking lockfile ts update.
+
+    RAISE_WINDOW_WAIT_STEPS = 10
+
+    def _release_lock(name):
+        """
+        Tries to remove a folder path.
+
+        :param name: folder lock to remove
+        :type name: str
+        """
+        try:
+            shutil.rmtree(name)
+            return True
+        except WindowsError as exc:
+            if exc.errno in (errno.EPIPE, errno.ENOENT,
+                             errno.ESRCH, errno.EACCES):
+                logger.warning(
+                    'exception while trying to remove the lockfile dir')
+                logger.warning('errno %s: %s' % (exc.errno, exc.args[1]))
+                # path does not exist
+                return False
+            else:
+                logger.debug('errno = %s' % (exc.errno,))
+                # we did not foresee this error, better add it explicitely
+                raise
+
     class WindowsLock(object):
         """
         Creates a lock based on the atomic nature of mkdir on Windows
@@ -200,7 +236,7 @@ if platform_init.IS_WIN:
 
         def get_pid(self):
             """
-            Returns the pid of the locking process
+            Returns the pid of the locking process.
 
             :rtype: int
             """
@@ -208,25 +244,31 @@ if platform_init.IS_WIN:
             _, pid = self._is_one_pidfile()
             return pid
 
-        def release_lock(self):
+        def get_locking_path(self):
+            """
+            Returns the pid path of the locking process.
+
+            :rtype: str
+            """
+            pid = self.get_pid()
+            if pid:
+                return "%s-%s" % (self.LOCKBASE, pid)
+
+        def release_lock(self, name=None):
             """
             Releases the pidfile dir for this process, by removing it.
             """
-            try:
-                shutil.rmtree(self.name)
-                return True
-            except WindowsError as exc:
-                if exc.errno in (errno.EPIPE, errno.ENOENT,
-                                 errno.ESRCH, errno.EACCES):
-                    logger.warning(
-                        'exception while trying to remove the lockfile dir')
-                    logger.warning('errno %s: %s' % (exc.errno, exc.args[1]))
-                    # path does not exist
-                    return False
-                else:
-                    logger.debug('errno = %s' % (exc.errno,))
-                    # we did not foresee this error, better add it explicitely
-                    raise
+            if not name:
+                name = self.name
+            _release_lock(name)
+
+        @classmethod
+        def release_all_locks(self):
+            """
+            Releases all locks. Used for clean shutdown.
+            """
+            for lockdir in glob.glob("%s-%s" % (self.LOCKBASE, '*')):
+                _release_lock(lockdir)
 
         @property
         def locked_by_us(self):
@@ -239,6 +281,13 @@ if platform_init.IS_WIN:
             _, pid = self._is_one_pidfile()
             return pid == self.pid
 
+        def update_ts(self):
+            """
+            Updates the timestamp of the lock.
+            """
+            if self.locked_by_us:
+                update_modification_ts(self.name)
+
         def write_port(self, port):
             """
             Writes the port for windows control to the pidfile folder
@@ -277,12 +326,27 @@ if platform_init.IS_WIN:
                     raise
             return port
 
+    def raise_window_ack():
+        """
+        This function is called from the windows callback that is registered
+        with the raise_window event. It just updates the modification time
+        of the lock file so we can signal an ack to the instance that tried
+        to raise the window.
+        """
+        lock = WindowsLock()
+        lock.update_ts()
+
 
 def we_are_the_one_and_only():
     """
     Returns True if we are the only instance running, False otherwise.
     If we came later, send a raise signal to the main instance of the
-    application
+    application.
+
+    Under windows we are not using flock magic, so we wait during
+    RAISE_WINDOW_TIMEOUT time, if not ack is
+    received, we assume it was a stalled lock, so we remove it and continue
+    with initialization.
 
     :rtype: bool
     """
@@ -300,9 +364,38 @@ def we_are_the_one_and_only():
         locker = WindowsLock()
         locker.get_lock()
         we_are_the_one = locker.locked_by_us
+
         if not we_are_the_one:
             locker.release_lock()
-            signal_event(proto.RAISE_WINDOW)
+        lock_path = locker.get_locking_path()
+        ts = get_modification_ts(lock_path)
+
+        nowfun = datetime.datetime.now
+        t0 = nowfun()
+        pause = RAISE_WINDOW_TIMEOUT / float(RAISE_WINDOW_WAIT_STEPS)
+        timeout_delta = datetime.timedelta(0, RAISE_WINDOW_TIMEOUT)
+        check_interval = lambda: nowfun() - t0 < timeout_delta
+
+        # let's assume it's a stalled lock
+        we_are_the_one = True
+        signal_event(proto.RAISE_WINDOW)
+
+        while check_interval():
+            if get_modification_ts(lock_path) > ts:
+                # yay! someone claimed their control over the lock.
+                # so the lock is alive
+                logger.debug('Raise window ACK-ed')
+                we_are_the_one = False
+                break
+            else:
+                time.sleep(pause)
+
+        if we_are_the_one:
+            # ok, it really was a stalled lock. let's remove all
+            # that is left, and put only ours there.
+            WindowsLock.release_all_locks()
+            WindowsLock().get_lock()
+
         return we_are_the_one
 
     else:
diff --git a/src/leap/services/eip/vpnlaunchers.py b/src/leap/services/eip/vpnlaunchers.py
index f031a6e5..fc77de48 100644
--- a/src/leap/services/eip/vpnlaunchers.py
+++ b/src/leap/services/eip/vpnlaunchers.py
@@ -535,8 +535,8 @@ class DarwinVPNLauncher(VPNLauncher):
     def get_cocoasudo_installmissing_cmd(self):
         """
         Returns a string with the cocoasudo command needed to install missing
-        files as admin with a nice password prompt. The actual command needs to be
-        appended.
+        files as admin with a nice password prompt. The actual command needs to
+        be appended.
 
         :rtype: (str, list)
         """
diff --git a/src/leap/util/__init__.py b/src/leap/util/__init__.py
index 5ceaede5..93eb714d 100644
--- a/src/leap/util/__init__.py
+++ b/src/leap/util/__init__.py
@@ -15,8 +15,10 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 """
-Initializes version and app info
+Initializes version and app info, plus some small and handy functions.
 """
+import datetime
+import os
 
 __version__ = "unknown"
 try:
@@ -47,3 +49,29 @@ def first(things):
         return things[0]
     except TypeError:
         return None
+
+
+def get_modification_ts(path):
+    """
+    Gets modification time of a file.
+
+    :param path: the path to get ts from
+    :type path: str
+    :returns: modification time
+    :rtype: datetime object
+    """
+    ts = os.path.getmtime(path)
+    return datetime.datetime.fromtimestamp(ts)
+
+
+def update_modification_ts(path):
+    """
+    Sets modification time of a file to current time.
+
+    :param path: the path to set ts to.
+    :type path: str
+    :returns: modification time
+    :rtype: datetime object
+    """
+    os.utime(path, None)
+    return get_modification_ts(path)