Repository: OpenAssetIO/OpenAssetIO (language: python)

Issue:
Add 'close' method to EntityReferencePagerInterface
In the PagerInterface, we currently say that the descruction of the object is the mechanism that the host signals that the query is over.

This has some problems that we should consider :
 - Does this encourage throwing in constructors?
 - Is overriding `del` pythonic
 - Is python destruction too unpredictable/non standard to rely on this.
 - Is it fine assuming that the manager implementer understands the lifetime implications of creating a pagerInterface, and having it be held alive by the pager object.
 
 The obvious alternate suggestion is to add a "closed" or "finished" or some other finalizing method to the pagerinterface, and call it in the destructor of pager.

_Triggered from a comment posted by @elliotcmorris in https://github.com/OpenAssetIO/OpenAssetIO/pull/986#discussion_r1238794887_


## ACs

- `close` method added to `PagerInterface`, called from destructor of `Pager` in try catch + error.
- Docs in interface state only called once and other methods wont be called, method must not throw.
- Default noop implementation
            


Discussion / hints:

Fix patch:
diff --git a/src/openassetio-core/CMakeLists.txt b/src/openassetio-core/CMakeLists.txt
index 89a6fd090..f5e69fc19 100644
--- a/src/openassetio-core/CMakeLists.txt
+++ b/src/openassetio-core/CMakeLists.txt
@@ -77,6 +77,7 @@ target_sources(
     src/managerApi/Host.cpp
     src/managerApi/HostSession.cpp
     src/managerApi/ManagerInterface.cpp
+    src/managerApi/EntityReferencePagerInterface.cpp
 )
 
 # Public header dependency.
diff --git a/src/openassetio-core/include/openassetio/hostApi/EntityReferencePager.hpp b/src/openassetio-core/include/openassetio/hostApi/EntityReferencePager.hpp
index a7518876d..da93751d6 100644
--- a/src/openassetio-core/include/openassetio/hostApi/EntityReferencePager.hpp
+++ b/src/openassetio-core/include/openassetio/hostApi/EntityReferencePager.hpp
@@ -82,7 +82,7 @@ class OPENASSETIO_CORE_EXPORT EntityReferencePager final {
   /**
    *  Destruction of this object is tantamount to closing the query.
    */
-  ~EntityReferencePager() = default;
+  ~EntityReferencePager();
 
   /**
    * Return whether or not there is more data accessible by advancing
diff --git a/src/openassetio-core/include/openassetio/managerApi/EntityReferencePagerInterface.hpp b/src/openassetio-core/include/openassetio/managerApi/EntityReferencePagerInterface.hpp
index e1374e094..52de299d4 100644
--- a/src/openassetio-core/include/openassetio/managerApi/EntityReferencePagerInterface.hpp
+++ b/src/openassetio-core/include/openassetio/managerApi/EntityReferencePagerInterface.hpp
@@ -113,6 +113,20 @@ class OPENASSETIO_CORE_EXPORT EntityReferencePagerInterface {
    * return an empty page.
    */
   virtual void next(const HostSessionPtr&) = 0;
+
+  /**
+   * Close the paging query.
+   *
+   * Signals that the host is finished with the paging query,
+   * allowing for any potential cleanup that may need to be performed.
+   * This method is guaranteed to be called only once, and no other
+   * interface methods will be called by the host thereafter.
+   *
+   * This method is called from a destructor. Thrown exceptions will be
+   * caught and logged as errors if possible. Despite that, throwing
+   * from this function is nonetheless discouraged.
+   */
+  virtual void close(const HostSessionPtr& hostSession);
 };
 static_assert(!std::is_copy_constructible_v<EntityReferencePagerInterface>);
 static_assert(!std::is_copy_assignable_v<EntityReferencePagerInterface>);
diff --git a/src/openassetio-core/src/hostApi/EntityReferencePager.cpp b/src/openassetio-core/src/hostApi/EntityReferencePager.cpp
index dbe99ce7d..b95aa4f24 100644
--- a/src/openassetio-core/src/hostApi/EntityReferencePager.cpp
+++ b/src/openassetio-core/src/hostApi/EntityReferencePager.cpp
@@ -3,7 +3,9 @@
 
 #include <openassetio/EntityReference.hpp>
 #include <openassetio/hostApi/EntityReferencePager.hpp>
+#include <openassetio/log/LoggerInterface.hpp>
 #include <openassetio/managerApi/EntityReferencePagerInterface.hpp>
+#include <openassetio/managerApi/HostSession.hpp>
 
 namespace openassetio {
 inline namespace OPENASSETIO_CORE_ABI_VERSION {
@@ -21,6 +23,17 @@ EntityReferencePager::EntityReferencePager(
     managerApi::HostSessionPtr hostSession)
     : pagerInterface_(std::move(pagerInterface)), hostSession_(std::move(hostSession)) {}
 
+EntityReferencePager::~EntityReferencePager() {
+  try {
+    pagerInterface_->close(hostSession_);
+  } catch (const std::exception& ex) {
+    hostSession_->logger()->error(ex.what());
+  } catch (...) {
+    hostSession_->logger()->error(
+        "Unknown non-exception object caught during destruction of EntityReferencePager");
+  }
+}
+
 bool EntityReferencePager::hasNext() { return pagerInterface_->hasNext(hostSession_); }
 
 typename EntityReferencePager::Page EntityReferencePager::get() {
diff --git a/src/openassetio-core/src/managerApi/EntityReferencePagerInterface.cpp b/src/openassetio-core/src/managerApi/EntityReferencePagerInterface.cpp
new file mode 100644
index 000000000..073c09641
--- /dev/null
+++ b/src/openassetio-core/src/managerApi/EntityReferencePagerInterface.cpp
@@ -0,0 +1,12 @@
+// SPDX-License-Identifier: Apache-2.0
+// Copyright 2013-2023 The Foundry Visionmongers Ltd
+#include <openassetio/managerApi/EntityReferencePagerInterface.hpp>
+
+namespace openassetio {
+inline namespace OPENASSETIO_CORE_ABI_VERSION {
+namespace managerApi {
+
+void EntityReferencePagerInterface::close([[maybe_unused]] const HostSessionPtr& hostSession) {}
+}  // namespace managerApi
+}  // namespace OPENASSETIO_CORE_ABI_VERSION
+}  // namespace openassetio
diff --git a/src/openassetio-python/cmodule/src/managerApi/EntityReferencePagerInterfaceBinding.cpp b/src/openassetio-python/cmodule/src/managerApi/EntityReferencePagerInterfaceBinding.cpp
index 55a8629e5..aead356a2 100644
--- a/src/openassetio-python/cmodule/src/managerApi/EntityReferencePagerInterfaceBinding.cpp
+++ b/src/openassetio-python/cmodule/src/managerApi/EntityReferencePagerInterfaceBinding.cpp
@@ -36,6 +36,10 @@ struct PyEntityReferencePagerInterface : EntityReferencePagerInterface {
   void next(const HostSessionPtr& hostSession) override {
     PYBIND11_OVERRIDE_PURE(void, EntityReferencePagerInterface, next, hostSession);
   }
+
+  void close(const HostSessionPtr& hostSession) override {
+    PYBIND11_OVERRIDE(void, EntityReferencePagerInterface, close, hostSession);
+  }
 };
 
 }  // namespace managerApi
@@ -52,5 +56,6 @@ void registerEntityReferencePagerInterface(const py::module& mod) {
       .def(py::init())
       .def("hasNext", &EntityReferencePagerInterface::hasNext, py::arg("hostSession").none(false))
       .def("get", &EntityReferencePagerInterface::get, py::arg("hostSession").none(false))
-      .def("next", &EntityReferencePagerInterface::next, py::arg("hostSession").none(false));
+      .def("next", &EntityReferencePagerInterface::next, py::arg("hostSession").none(false))
+      .def("close", &EntityReferencePagerInterface::close, py::arg("hostSession").none(false));
 }


Test patch:
diff --git a/src/openassetio-python/tests/conftest.py b/src/openassetio-python/tests/conftest.py
index 038e650c1..e7dafcead 100644
--- a/src/openassetio-python/tests/conftest.py
+++ b/src/openassetio-python/tests/conftest.py
@@ -446,6 +446,9 @@ def get(self, hostSession):
     def next(self, hostSession):
         self.mock.next(hostSession)
 
+    def close(self, hostSession):
+        self.mock.close(hostSession)
+
 
 #
 # Python to C++ migration helpers
diff --git a/src/openassetio-python/tests/package/hostApi/test_entityreferencepager.py b/src/openassetio-python/tests/package/hostApi/test_entityreferencepager.py
index 2b2bbd159..ffa79e581 100644
--- a/src/openassetio-python/tests/package/hostApi/test_entityreferencepager.py
+++ b/src/openassetio-python/tests/package/hostApi/test_entityreferencepager.py
@@ -27,6 +27,7 @@
 
 from openassetio import EntityReference
 from openassetio.hostApi import Manager, EntityReferencePager
+from openassetio.log import LoggerInterface
 from openassetio.managerApi import EntityReferencePagerInterface
 
 
@@ -137,6 +138,33 @@ def makePagerInScope():
         # destroyed.
         assert weak_interface_ref() is None
 
+    def test_when_EntityReferencePager_destructed_close_is_called(
+        self, a_host_session, mock_entity_reference_pager_interface
+    ):
+        pager = EntityReferencePager(mock_entity_reference_pager_interface, a_host_session)
+        del pager
+        mock_entity_reference_pager_interface.mock.close.assert_called_once_with(a_host_session)
+
+    def test_when_exception_thrown_in_close_exception_is_caught_and_logged(
+        self, a_host_session, mock_entity_reference_pager_interface, mock_logger
+    ):
+        exception_what = "Mocked exception"
+
+        def raise_exception(self):
+            raise Exception(exception_what)
+
+        mock_entity_reference_pager_interface.mock.close.side_effect = raise_exception
+
+        pager = EntityReferencePager(mock_entity_reference_pager_interface, a_host_session)
+        del pager
+        mock_entity_reference_pager_interface.mock.close.assert_called_once_with(a_host_session)
+        args, kwargs = mock_logger.mock.log.call_args
+
+        # The .what() of the exception comes with a lot of additional
+        # text about call location that would be overly verbose to check
+        assert LoggerInterface.Severity.kError == args[0]
+        assert exception_what in args[1]
+
 
 @pytest.fixture
 def an_entity_reference_pager(mock_entity_reference_pager_interface, a_host_session):