[4.4] Deprecate timeout config options introduced in 4.4.5 (#768)

 * `update_routing_table_timeout`
 * `session_connection_timeout`

Server-side keep-alives communicated through configuration hints together with
`connection_acquisition_timeout` (which, starting with 4.4.5, includes the full
handshake, i.e., with `HELLO` message) are sufficient to avoid the driver
getting stuck.

Author: robsdedude

docs/source/api.rst

@@ -194,6 +194,11 @@ larger than :ref:`update-routing-table-timeout-ref`,
 
 .. versionadded:: 4.4.5
 
+.. deprecated:: 4.4.6
+    Will be removed in 5.0. Use server-side bolt-keep-alive together with
+    :ref:`connection-acquisition-timeout-ref` instead to ensure the driver
+    cannot hang.
+
 
 .. _update-routing-table-timeout-ref:
 
@@ -216,6 +221,11 @@ This setting only has an effect for :ref:`neo4j-driver-ref`, but not for
 
 .. versionadded:: 4.4.5
 
+.. deprecated:: 4.4.6
+    Will be removed in 5.0. Use server-side bolt-keep-alive together with
+    :ref:`connection-acquisition-timeout-ref` instead to ensure the driver
+    cannot hang.
+
 
 .. _connection-acquisition-timeout-ref:
 

neo4j/conf.py

@@ -21,9 +21,13 @@
 
 from abc import ABCMeta
 from collections.abc import Mapping
+import warnings
 from warnings import warn
 
-from neo4j.meta import get_user_agent
+from neo4j.meta import (
+    deprecation_warn,
+    get_user_agent,
+)
 
 from neo4j.api import (
     TRUST_SYSTEM_CA_SIGNED_CERTIFICATES,
@@ -57,43 +61,58 @@ def __init__(self, new):
         self.new = new
 
 
+class DeprecatedOption:
+
+    def __init__(self, value):
+        self.value = value
+
+
 class ConfigType(ABCMeta):
 
     def __new__(mcs, name, bases, attributes):
         fields = []
         deprecated_aliases = {}
+        deprecated_options = {}
 
         for base in bases:
             if type(base) is mcs:
                 fields += base.keys()
                 deprecated_aliases.update(base._deprecated_aliases())
+                deprecated_options.update(base._deprecated_options())
 
         for k, v in attributes.items():
             if isinstance(v, DeprecatedAlias):
                 deprecated_aliases[k] = v.new
             elif not k.startswith("_") and not callable(v):
                 fields.append(k)
+            if isinstance(v, DeprecatedOption):
+                deprecated_options[k] = v.value
+                attributes[k] = v.value
 
         def keys(_):
-            return fields
+            return set(fields)
 
         def _deprecated_aliases(_):
             return deprecated_aliases
 
+        def _deprecated_options(_):
+            return deprecated_options
+
         def _deprecated_keys(_):
-            return list(deprecated_aliases)
+            return set(deprecated_aliases.keys())
 
         def _get_new(_, key):
             return deprecated_aliases.get(key)
 
         attributes.setdefault("keys", classmethod(keys))
         attributes.setdefault("_deprecated_aliases", classmethod(_deprecated_aliases))
+        attributes.setdefault("_deprecated_options", classmethod(_deprecated_options))
         attributes.setdefault("_deprecated_keys", classmethod(_deprecated_keys))
         attributes.setdefault("_get_new", classmethod(_get_new))
 
         return super(ConfigType, mcs).__new__(mcs, name, bases,
                                               {k: v for k, v in attributes.items()
-                                               if k not in deprecated_aliases})
+                                               if k not in _deprecated_keys(None)})
 
 
 class Config(Mapping, metaclass=ConfigType):
@@ -120,7 +139,7 @@ def consume(cls, data):
     def _consume(cls, data):
         config = {}
         if data:
-            for key in list(cls.keys()) + list(cls._deprecated_keys()):
+            for key in cls.keys() | cls._deprecated_keys():
                 try:
                     value = data.pop(key)
                 except KeyError:
@@ -134,12 +153,19 @@ def __update(self, data):
 
         def set_attr(k, v):
             if k in self.keys():
+                if k in self._deprecated_options():
+                    deprecation_warn("The '{}' config key is "
+                                     "deprecated".format(k))
                 setattr(self, k, v)
             elif k in self._deprecated_keys():
                 k0 = self._get_new(k)
                 if k0 in data_dict:
-                    raise ValueError("Cannot specify both '{}' and '{}' in config".format(k0, k))
-                warn("The '{}' config key is deprecated, please use '{}' instead".format(k, k0))
+                    raise ValueError("Cannot specify both '{}' and '{}' in "
+                                     "config".format(k0, k))
+                deprecation_warn(
+                    "The '{}' config key is deprecated, please use "
+                    "'{}' instead".format(k, k0)
+                )
                 set_attr(k0, v)
             else:
                 raise AttributeError(k)
@@ -150,7 +176,13 @@ def set_attr(k, v):
 
     def __init__(self, *args, **kwargs):
         for arg in args:
-            self.__update(arg)
+            if isinstance(arg, Config):
+                with warnings.catch_warnings():
+                    warnings.filterwarnings("ignore",
+                                            category=DeprecationWarning)
+                    self.__update(arg)
+            else:
+                self.__update(arg)
         self.__update(kwargs)
 
     def __repr__(self):
@@ -186,7 +218,7 @@ class PoolConfig(Config):
     # The maximum amount of time to wait for a TCP connection to be established.
 
     #: Update Routing Table Timout
-    update_routing_table_timeout = 90.0  # seconds
+    update_routing_table_timeout = DeprecatedOption(90.0)  # seconds
     # The maximum amount of time to wait for updating the routing table.
     # This includes everything necessary for this to happen.
     # Including opening sockets, requesting and receiving the routing table,
@@ -264,7 +296,7 @@ class WorkspaceConfig(Config):
     """
 
     #: Session Connection Timeout
-    session_connection_timeout = float("inf")  # seconds
+    session_connection_timeout = DeprecatedOption(float("inf"))  # seconds
     # The maximum amount of time to wait for a session to obtain a usable
     # read/write connection. This includes everything necessary for this to
     # happen. Including fetching routing tables, opening sockets, etc.

tests/unit/test_conf.py

@@ -19,6 +19,9 @@
 # limitations under the License.
 
 
+from contextlib import contextmanager
+import warnings
+
 import pytest
 
 from neo4j.exceptions import (
@@ -72,11 +75,26 @@
 config_function_names = ["consume_chain", "consume"]
 
 
+@contextmanager
+def _pool_config_deprecations():
+    with pytest.warns(DeprecationWarning,
+                      match="update_routing_table_timeout") as warnings:
+        yield warnings
+
+
+@contextmanager
+def _session_config_deprecations():
+    with pytest.warns(DeprecationWarning,
+                      match="session_connection_timeout") as warnings:
+        yield warnings
+
+
 def test_pool_config_consume():
 
     test_config = dict(test_pool_config)
 
-    consumed_pool_config = PoolConfig.consume(test_config)
+    with _pool_config_deprecations():
+        consumed_pool_config = PoolConfig.consume(test_config)
 
     assert isinstance(consumed_pool_config, PoolConfig)
 
@@ -89,7 +107,8 @@ def test_pool_config_consume():
         if key not in config_function_names:
             assert test_pool_config[key] == consumed_pool_config[key]
 
-    assert len(consumed_pool_config) - len(config_function_names) == len(test_pool_config)
+    assert (len(consumed_pool_config) - len(config_function_names)
+            == len(test_pool_config))
 
 
 def test_pool_config_consume_default_values():
@@ -114,7 +133,11 @@ def test_pool_config_consume_key_not_valid():
     test_config["not_valid_key"] = "test"
 
     with pytest.raises(ConfigurationError) as error:
-        consumed_pool_config = PoolConfig.consume(test_config)
+        # might or might not warn DeprecationWarning, but we're only
+        # interested in the error
+        with warnings.catch_warnings():
+            warnings.filterwarnings("ignore", category=DeprecationWarning)
+            _ = PoolConfig.consume(test_config)
 
     error.match("Unexpected config keys: not_valid_key")
 
@@ -123,7 +146,8 @@ def test_pool_config_set_value():
 
     test_config = dict(test_pool_config)
 
-    consumed_pool_config = PoolConfig.consume(test_config)
+    with _pool_config_deprecations():
+        consumed_pool_config = PoolConfig.consume(test_config)
 
     assert consumed_pool_config.get("encrypted") is False
     assert consumed_pool_config["encrypted"] is False
@@ -141,28 +165,37 @@ def test_pool_config_set_value():
 def test_pool_config_consume_and_then_consume_again():
 
     test_config = dict(test_pool_config)
-    consumed_pool_config = PoolConfig.consume(test_config)
+    with _pool_config_deprecations():
+        consumed_pool_config = PoolConfig.consume(test_config)
     assert consumed_pool_config.encrypted is False
     consumed_pool_config.encrypted = "test"
 
     with pytest.raises(AttributeError):
-        consumed_pool_config = PoolConfig.consume(consumed_pool_config)
+        _ = PoolConfig.consume(consumed_pool_config)
 
-    consumed_pool_config = PoolConfig.consume(dict(consumed_pool_config.items()))
-    consumed_pool_config = PoolConfig.consume(dict(consumed_pool_config.items()))
+    with _pool_config_deprecations():
+        consumed_pool_config = PoolConfig.consume(
+            dict(consumed_pool_config.items())
+        )
+    with _pool_config_deprecations():
+        consumed_pool_config = PoolConfig.consume(
+            dict(consumed_pool_config.items())
+        )
 
     assert consumed_pool_config.encrypted == "test"
 
 
 def test_config_consume_chain():
 
     test_config = {}
-
     test_config.update(test_pool_config)
-
     test_config.update(test_session_config)
 
-    consumed_pool_config, consumed_session_config = Config.consume_chain(test_config, PoolConfig, SessionConfig)
+    with warnings.catch_warnings():
+        warnings.filterwarnings("ignore", category=DeprecationWarning)
+        consumed_pool_config, consumed_session_config = Config.consume_chain(
+            test_config, PoolConfig, SessionConfig
+        )
 
     assert isinstance(consumed_pool_config, PoolConfig)
     assert isinstance(consumed_session_config, SessionConfig)
@@ -176,9 +209,11 @@ def test_config_consume_chain():
         if key not in config_function_names:
             assert test_pool_config[key] == val
 
-    assert len(consumed_pool_config) - len(config_function_names) == len(test_pool_config)
+    assert (len(consumed_pool_config) - len(config_function_names)
+            == len(test_pool_config))
 
-    assert len(consumed_session_config) - len(config_function_names) == len(test_session_config)
+    assert (len(consumed_session_config) - len(config_function_names)
+            == len(test_session_config))
 
 
 def test_init_session_config_merge():
@@ -226,3 +261,21 @@ def test_init_session_config_with_not_valid_key():
         _ = SessionConfig.consume(test_config_b)
 
     assert session_config.connection_acquisition_timeout == 333
+
+
+def test_pool_config_deprecated_update_routing_table_timeout():
+    with _pool_config_deprecations():
+        _ = PoolConfig.consume({"update_routing_table_timeout": 1})
+
+    with warnings.catch_warnings():
+        warnings.simplefilter("error")
+        _ = PoolConfig.consume({})
+
+
+def test_session_config_deprecated_session_connection_timeout():
+    with _session_config_deprecations():
+        _ = SessionConfig.consume({"session_connection_timeout": 1})
+
+    with warnings.catch_warnings():
+        warnings.simplefilter("error")
+        _ = SessionConfig.consume({})