Fix connection reconciliation to handle restarts (#882)

Author: kevin-bates

jupyter_client/connect.py

@@ -574,7 +574,8 @@ def _reconcile_connection_info(self, info: KernelConnectionInfo) -> None:
         Because some provisioners (like derivations of LocalProvisioner) may have already
         written the connection file, this method needs to ensure that, if the connection
         file exists, its contents match that of what was returned by the provisioner.  If
-        the file does exist and its contents do not match, a ValueError is raised.
+        the file does exist and its contents do not match, the file will be replaced with
+        the provisioner information (which is considered the truth).
 
         If the file does not exist, the connection information in 'info' is loaded into the
         KernelManager and written to the file.
@@ -587,24 +588,24 @@ def _reconcile_connection_info(self, info: KernelConnectionInfo) -> None:
         if os.path.exists(self.connection_file):
             with open(self.connection_file) as f:
                 file_info = json.load(f)
-                # Prior to the following comparison, we need to adjust the value of "key" to
-                # be bytes, otherwise the comparison below will fail.
-                file_info["key"] = file_info["key"].encode()
-                if not self._equal_connections(info, file_info):
-                    raise ValueError(
-                        "Connection file already exists and does not match "
-                        "the expected values returned from provisioner!"
-                    )
+            # Prior to the following comparison, we need to adjust the value of "key" to
+            # be bytes, otherwise the comparison below will fail.
+            file_info["key"] = file_info["key"].encode()
+            if not self._equal_connections(info, file_info):
+                os.remove(self.connection_file)  # Contents mismatch - remove the file
+                self._connection_file_written = False
+            else:
                 file_exists = True
 
         if not file_exists:
-            # Load the connection info and write out file. Note, this does not necessarily
-            # overwrite non-zero port values, so we'll validate afterward.
+            # Load the connection info and write out file, clearing existing
+            # port-based attributes so they will be reloaded
+            for name in port_names:
+                setattr(self, name, 0)
             self.load_connection_info(info)
             self.write_connection_file()
 
-        # Ensure what is in KernelManager is what we expect.  This will catch issues if the file
-        # already existed, yet it's contents differed from the KernelManager's (and provisioner).
+        # Ensure what is in KernelManager is what we expect.
         km_info = self.get_connection_info()
         if not self._equal_connections(info, km_info):
             raise ValueError(

tests/test_connect.py

@@ -240,15 +240,15 @@ def test_mixin_cleanup_random_ports():
 
 
 param_values = [
-    (True, True, None),
-    (True, False, ValueError),
-    (False, True, None),
-    (False, False, ValueError),
+    (True, True),
+    (True, False),
+    (False, True),
+    (False, False),
 ]
 
 
-@pytest.mark.parametrize("file_exists, km_matches, expected_exception", param_values)
-def test_reconcile_connection_info(file_exists, km_matches, expected_exception):
+@pytest.mark.parametrize("file_exists, km_matches", param_values)
+def test_reconcile_connection_info(file_exists, km_matches):
 
     expected_info = sample_info
     mismatched_info = sample_info.copy()
@@ -272,9 +272,10 @@ def test_reconcile_connection_info(file_exists, km_matches, expected_exception):
                 provisioner_info = info
                 km.load_connection_info(provisioner_info)
             else:
-                # Let this be the case where the connection file exists, the KM has no values
-                # prior to reconciliation, but the provisioner has returned different values
-                # and a ValueError is expected.
+                # Let this be the case where the connection file exists, and the KM has those values
+                # that differ from the ones returned by the provisioner.  This is the restart-with-
+                # changed-ports case (typical for remote provisioners).
+                km.load_connection_info(expected_info)
                 provisioner_info = mismatched_info
         else:  # connection file does not exist
             if km_matches:
@@ -284,20 +285,11 @@ def test_reconcile_connection_info(file_exists, km_matches, expected_exception):
                 provisioner_info = expected_info
             else:
                 # Let this be the case where the connection file does not exist, yet the KM
-                # has values that do not match those returned from the provisioner and a
-                # ValueError is expected.
+                # has values that do not match those returned from the provisioner.  This case
+                # is probably not practical and is equivalent to the True, False case.
                 km.load_connection_info(expected_info)
                 provisioner_info = mismatched_info
 
-        if expected_exception is None:
-            km._reconcile_connection_info(provisioner_info)
-            km_info = km.get_connection_info()
-            assert km._equal_connections(km_info, provisioner_info)
-        else:
-            with pytest.raises(expected_exception) as ex:
-                km._reconcile_connection_info(provisioner_info)
-            if file_exists:
-                assert "Connection file already exists" in str(ex.value)
-            else:
-                assert "KernelManager's connection information already exists" in str(ex.value)
-            assert km._equal_connections(km.get_connection_info(), provisioner_info) is False
+        km._reconcile_connection_info(provisioner_info)
+        km_info = km.get_connection_info()
+        assert km._equal_connections(km_info, provisioner_info)