Repository: jupyter/jupyter_client
Pull request #882 (2022-11-16 00:03:04)

Fix connection reconciliation to handle restarts
The changes delivered in #879 did not account for scenarios when ports have changed during restarts - which tends to occur with remote kernels.  As a result, we need to reset the connection file when the connection information returned from the provisioner differs from the file's contents, rather than raise a `ValueError`.  

Although this is probably not an issue in typical situations, it is something that should be fixed.  I apologize to @blink1073 for having not caught this in my initial changes in #879 since two different releases were cut following those changes.  


Patch:
diff --git a/jupyter_client/connect.py b/jupyter_client/connect.py
index 97bab17..8ebe96c 100644
--- a/jupyter_client/connect.py
+++ b/jupyter_client/connect.py
@@ -574,7 +574,8 @@ class ConnectionFileMixin(LoggingConfigurable):
         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 @@ class ConnectionFileMixin(LoggingConfigurable):
         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(


Test patch:
diff --git a/tests/test_connect.py b/tests/test_connect.py
index 7103cf2..4449b90 100644
--- a/tests/test_connect.py
+++ b/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)