From 5407a60c79301e73559207162b9dc5b1ae0e559d Mon Sep 17 00:00:00 2001 From: Bernard Normier Date: Wed, 30 Oct 2024 15:01:25 -0400 Subject: [PATCH] Specify connection when creating Allocation/Admin session for Glacier2 (#2966) --- cpp/src/IceGrid/AdminSessionI.cpp | 20 +++++++++++++------- cpp/src/IceGrid/AdminSessionI.h | 7 +++++-- cpp/src/IceGrid/InternalRegistryI.cpp | 10 ++++++++-- cpp/src/IceGrid/NodeSessionI.cpp | 6 +++--- cpp/src/IceGrid/NodeSessionI.h | 2 +- cpp/src/IceGrid/ReapThread.cpp | 18 +++++++----------- cpp/src/IceGrid/ReapThread.h | 25 ++++++++++++++++++++++--- cpp/src/IceGrid/RegistryI.cpp | 18 +++++++----------- cpp/src/IceGrid/ReplicaSessionI.cpp | 6 +++--- cpp/src/IceGrid/ReplicaSessionI.h | 2 +- cpp/src/IceGrid/SessionI.cpp | 24 +++++++++++++----------- cpp/src/IceGrid/SessionI.h | 10 ++++++---- 12 files changed, 89 insertions(+), 59 deletions(-) diff --git a/cpp/src/IceGrid/AdminSessionI.cpp b/cpp/src/IceGrid/AdminSessionI.cpp index 7314edf5800..44f9ef5058b 100644 --- a/cpp/src/IceGrid/AdminSessionI.cpp +++ b/cpp/src/IceGrid/AdminSessionI.cpp @@ -459,12 +459,15 @@ AdminSessionFactory::AdminSessionFactory( } Glacier2::SessionPrx -AdminSessionFactory::createGlacier2Session(const string& sessionId, const optional& ctl) +AdminSessionFactory::createGlacier2Session( + const string& sessionId, + const optional& ctl, + const Ice::ConnectionPtr& con) { assert(_servantManager); auto session = createSessionServant(sessionId); - auto proxy = session->_register(_servantManager, nullptr); + auto proxy = session->_register(_servantManager, con); if (ctl) { @@ -489,7 +492,7 @@ AdminSessionFactory::createGlacier2Session(const string& sessionId, const option // We can't use a non-0 timeout such as the Glacier2 session timeout. As of Ice 3.8, heartbeats may not be sent // at all on a busy connection. Furthermore, as of Ice 3.8, Glacier2 no longer "converts" heartbeats into // keepAlive requests. - _reaper->add(make_shared>(_database->getTraceLevels()->logger, session), 0s); + _reaper->add(make_shared>(_database->getTraceLevels()->logger, session), 0s, con); return Ice::uncheckedCast(proxy); } @@ -508,15 +511,18 @@ AdminSessionFactory::getTraceLevels() const AdminSessionManagerI::AdminSessionManagerI(const shared_ptr& factory) : _factory(factory) {} optional -AdminSessionManagerI::create(string userId, optional ctl, const Ice::Current&) +AdminSessionManagerI::create(string userId, optional ctl, const Ice::Current& current) { - return _factory->createGlacier2Session(std::move(userId), std::move(ctl)); + return _factory->createGlacier2Session(std::move(userId), std::move(ctl), current.con); } AdminSSLSessionManagerI::AdminSSLSessionManagerI(const shared_ptr& factory) : _factory(factory) {} optional -AdminSSLSessionManagerI::create(Glacier2::SSLInfo info, optional ctl, const Ice::Current&) +AdminSSLSessionManagerI::create( + Glacier2::SSLInfo info, + optional ctl, + const Ice::Current& current) { string userDN; if (!info.certs.empty()) // TODO: Require userDN? @@ -535,5 +541,5 @@ AdminSSLSessionManagerI::create(Glacier2::SSLInfo info, optionalcreateGlacier2Session(std::move(userDN), std::move(ctl)); + return _factory->createGlacier2Session(std::move(userDN), std::move(ctl), current.con); } diff --git a/cpp/src/IceGrid/AdminSessionI.h b/cpp/src/IceGrid/AdminSessionI.h index 26baac27cb3..8c22877b1a9 100644 --- a/cpp/src/IceGrid/AdminSessionI.h +++ b/cpp/src/IceGrid/AdminSessionI.h @@ -86,8 +86,11 @@ namespace IceGrid const std::shared_ptr&, const std::shared_ptr&); - Glacier2::SessionPrx - createGlacier2Session(const std::string&, const std::optional&); + Glacier2::SessionPrx createGlacier2Session( + const std::string& sessionId, + const std::optional& ctl, + const Ice::ConnectionPtr& con); + std::shared_ptr createSessionServant(const std::string&); const std::shared_ptr& getTraceLevels() const; diff --git a/cpp/src/IceGrid/InternalRegistryI.cpp b/cpp/src/IceGrid/InternalRegistryI.cpp index 9f84c459991..1fbff816b97 100644 --- a/cpp/src/IceGrid/InternalRegistryI.cpp +++ b/cpp/src/IceGrid/InternalRegistryI.cpp @@ -62,7 +62,10 @@ InternalRegistryI::registerNode( try { auto session = NodeSessionI::create(_database, std::move(*node), info, _nodeSessionTimeout, load); - _reaper->add(make_shared>(logger, session), _nodeSessionTimeout); + + // nullptr for the connection parameter, meaning the reaper won't destroy the session if the connection is + // closed. + _reaper->add(make_shared>(logger, session), _nodeSessionTimeout, nullptr); return session->getProxy(); } catch (const Ice::ObjectAdapterDestroyedException&) @@ -93,7 +96,10 @@ InternalRegistryI::registerReplica( try { auto s = ReplicaSessionI::create(_database, _wellKnownObjects, info, std::move(*prx), _replicaSessionTimeout); - _reaper->add(make_shared>(logger, s), _replicaSessionTimeout); + + // nullptr for the connection parameter, meaning the reaper won't destroy the session if the connection is + // closed. + _reaper->add(make_shared>(logger, s), _replicaSessionTimeout, nullptr); return s->getProxy(); } catch (const Ice::ObjectAdapterDestroyedException&) diff --git a/cpp/src/IceGrid/NodeSessionI.cpp b/cpp/src/IceGrid/NodeSessionI.cpp index 16e2f0da3af..bf625a75921 100644 --- a/cpp/src/IceGrid/NodeSessionI.cpp +++ b/cpp/src/IceGrid/NodeSessionI.cpp @@ -186,13 +186,13 @@ NodeSessionI::destroy(const Ice::Current&) destroyImpl(false); } -chrono::steady_clock::time_point -NodeSessionI::timestamp() const +optional +NodeSessionI::timestamp() const noexcept { lock_guard lock(_mutex); if (_destroy) { - throw Ice::ObjectNotExistException{__FILE__, __LINE__}; + return nullopt; } return _timestamp; } diff --git a/cpp/src/IceGrid/NodeSessionI.h b/cpp/src/IceGrid/NodeSessionI.h index 77d80e5dbc0..3d7a93d272c 100644 --- a/cpp/src/IceGrid/NodeSessionI.h +++ b/cpp/src/IceGrid/NodeSessionI.h @@ -38,7 +38,7 @@ namespace IceGrid const Ice::Current&) const final; void destroy(const Ice::Current&) final; - std::chrono::steady_clock::time_point timestamp() const; + std::optional timestamp() const noexcept; void shutdown(); const NodePrx& getNode() const; diff --git a/cpp/src/IceGrid/ReapThread.cpp b/cpp/src/IceGrid/ReapThread.cpp index 86fca1ffc77..7dba7324ea0 100644 --- a/cpp/src/IceGrid/ReapThread.cpp +++ b/cpp/src/IceGrid/ReapThread.cpp @@ -50,25 +50,19 @@ ReapThread::run() auto p = _sessions.begin(); while (p != _sessions.end()) { - try + if (auto timestamp = p->item->timestamp()) { - auto timestamp = p->item->timestamp(); // throws ONE if the reapable is destroyed. - - if (p->timeout > 0s && (chrono::steady_clock::now() - timestamp > p->timeout)) + if (p->timeout > 0s && (chrono::steady_clock::now() - *timestamp > p->timeout)) { reap.push_back(*p); - // and go to the code after the catch block } else { ++p; - continue; + continue; // while loop } } - catch (const Ice::ObjectNotExistException&) - { - // already destroyed - } + // else session is already destroyed and we clean-up // Remove the reapable if (p->connection) @@ -141,7 +135,7 @@ ReapThread::add(const shared_ptr& reapable, chrono::seconds timeout, c } // NOTE: registering a reapable with a 0s timeout is allowed. The reapable is reaped only when the reaper thread is - // shutdown or the connection is closed. + // shutdown or the connection is closed (when connection is not null). // // 10 seconds is the minimum permissable timeout (for non-zero timeouts). @@ -155,6 +149,8 @@ ReapThread::add(const shared_ptr& reapable, chrono::seconds timeout, c if (connection) { + assert(timeout == 0s); + auto p = _connections.find(connection); if (p == _connections.end()) { diff --git a/cpp/src/IceGrid/ReapThread.h b/cpp/src/IceGrid/ReapThread.h index 076bb41794e..facdbf962bc 100644 --- a/cpp/src/IceGrid/ReapThread.h +++ b/cpp/src/IceGrid/ReapThread.h @@ -17,7 +17,8 @@ namespace IceGrid public: virtual ~Reapable() = default; - virtual std::chrono::steady_clock::time_point timestamp() const = 0; + // Returns the timestamp of the most recent keepAlive call, or nullopt if the session is destroyed. + virtual std::optional timestamp() const noexcept = 0; virtual void destroy(bool) = 0; }; @@ -32,7 +33,10 @@ namespace IceGrid { } - std::chrono::steady_clock::time_point timestamp() const final { return _session->timestamp(); } + std::optional timestamp() const noexcept final + { + return _session->timestamp(); + } void destroy(bool shutdown) final { @@ -62,6 +66,15 @@ namespace IceGrid const std::shared_ptr _session; }; + // A shared monitoring/reaping mechanism that destroys session servants. It's used for all session servants hosted + // by the IceGrid registry: admin sessions, client sessions (aka resource allocation sessions), internal node + // sessions and internal replica sessions. + // It supports two modes: + // - 0s timeout + non-null connection: the session is bound to the connection and is destroyed/reaped either + // explicitly (via a call to a Slice-defined destroy operation) or when the connection is closed + // - a null connection with usually a non-0 timeout: the session's lifetime is independent of the connection, and + // the reaper destroys the session when it doesn't receive a keepAlive call within timeout. If the timeout is 0s, + // the reaper only reaps the session when it's destroyed explicitly, or when the IceGrid registry shuts down. class ReapThread final { public: @@ -69,7 +82,13 @@ namespace IceGrid void terminate(); void join(); - void add(const std::shared_ptr&, std::chrono::seconds, const Ice::ConnectionPtr& = nullptr); + + // Add a session (wrapped in a Reapable object), with a specified timeout (can be 0s) and connection (can be + // null). + void + add(const std::shared_ptr& reapable, + std::chrono::seconds timeout, + const Ice::ConnectionPtr& connection); void connectionClosed(const Ice::ConnectionPtr&); diff --git a/cpp/src/IceGrid/RegistryI.cpp b/cpp/src/IceGrid/RegistryI.cpp index 19604b4f93c..05849b27ee6 100644 --- a/cpp/src/IceGrid/RegistryI.cpp +++ b/cpp/src/IceGrid/RegistryI.cpp @@ -1073,20 +1073,16 @@ RegistryI::createAdminSessionFromSecureConnection(const Current& current) int RegistryI::getSessionTimeout(const Ice::Current&) const { + // getSessionTimeout is called by clients that create sessions (resource allocation sessions aka client sessions and + // admin sessions) directly using the IceGrid::Registry interface. These sessions are hosted by the + // IceGrid.Registry.Client object adapter. + + // A Glacier2 client that creates a session using the Glacier2::SessionManager can't call this operation since it + // doesn't have access to the IceGrid::Registry interface. PropertiesPtr properties = _communicator->getProperties(); int serverIdleTimeout = properties->getIcePropertyAsInt("Ice.Connection.Server.IdleTimeout"); - int adminSessionTimeout = properties->getPropertyAsIntWithDefault( - "IceGrid.Registry.AdminSessionManager.Connection.IdleTimeout", - serverIdleTimeout); - int sessionTimeout = properties->getPropertyAsIntWithDefault( - "IceGrid.Registry.SessionManager.Connection.IdleTimeout", - serverIdleTimeout); - - // Users should not fine-tune the idle timeout so both timeouts are usually identical. In case they are different, - // we return the min to the caller (an old Ice client, with Ice version <= 3.7). The caller may then send keep alive - // more often than necessary (very minor). - return min(adminSessionTimeout, sessionTimeout); + return properties->getPropertyAsIntWithDefault("IceGrid.Registry.Client.Connection.IdleTimeout", serverIdleTimeout); } string diff --git a/cpp/src/IceGrid/ReplicaSessionI.cpp b/cpp/src/IceGrid/ReplicaSessionI.cpp index 05da3947105..9d0c647e87b 100644 --- a/cpp/src/IceGrid/ReplicaSessionI.cpp +++ b/cpp/src/IceGrid/ReplicaSessionI.cpp @@ -249,13 +249,13 @@ ReplicaSessionI::destroy(const Ice::Current&) destroyImpl(false); } -chrono::steady_clock::time_point -ReplicaSessionI::timestamp() const +optional +ReplicaSessionI::timestamp() const noexcept { lock_guard lock(_mutex); if (_destroy) { - throw Ice::ObjectNotExistException{__FILE__, __LINE__}; + return nullopt; } return _timestamp; } diff --git a/cpp/src/IceGrid/ReplicaSessionI.h b/cpp/src/IceGrid/ReplicaSessionI.h index 4e421aa783e..db498d04877 100644 --- a/cpp/src/IceGrid/ReplicaSessionI.h +++ b/cpp/src/IceGrid/ReplicaSessionI.h @@ -35,7 +35,7 @@ namespace IceGrid void receivedUpdate(TopicName, int, std::string, const Ice::Current&) override; void destroy(const Ice::Current&) override; - std::chrono::steady_clock::time_point timestamp() const; + std::optional timestamp() const noexcept; void shutdown(); const InternalRegistryPrx& getInternalRegistry() const; diff --git a/cpp/src/IceGrid/SessionI.cpp b/cpp/src/IceGrid/SessionI.cpp index a9e7798e9bc..dd64df81f98 100644 --- a/cpp/src/IceGrid/SessionI.cpp +++ b/cpp/src/IceGrid/SessionI.cpp @@ -79,14 +79,13 @@ BaseSessionI::destroyImpl(bool) } } -std::chrono::steady_clock::time_point -BaseSessionI::timestamp() const +optional +BaseSessionI::timestamp() const noexcept { lock_guard lock(_mutex); if (_destroyed) { - // Just a "marker" exception here. - throw Ice::ObjectNotExistException{__FILE__, __LINE__}; + return nullopt; } return std::chrono::steady_clock::time_point::min(); // not used } @@ -277,12 +276,15 @@ ClientSessionFactory::ClientSessionFactory( } Glacier2::SessionPrx -ClientSessionFactory::createGlacier2Session(const string& sessionId, const optional& ctl) +ClientSessionFactory::createGlacier2Session( + const string& sessionId, + const optional& ctl, + const Ice::ConnectionPtr& con) { assert(_servantManager); auto session = createSessionServant(sessionId); - auto proxy = session->_register(_servantManager, nullptr); + auto proxy = session->_register(_servantManager, con); if (ctl) { @@ -308,7 +310,7 @@ ClientSessionFactory::createGlacier2Session(const string& sessionId, const optio // We can't use a non-0 timeout such as the Glacier2 session timeout. As of Ice 3.8, heartbeats may not be sent // at all on a busy connection. Furthermore, as of Ice 3.8, Glacier2 no longer "converts" heartbeats into // keepAlive requests. - _reaper->add(make_shared>(_database->getTraceLevels()->logger, session), 0s); + _reaper->add(make_shared>(_database->getTraceLevels()->logger, session), 0s, con); return Ice::uncheckedCast(proxy); } @@ -327,9 +329,9 @@ ClientSessionFactory::getTraceLevels() const ClientSessionManagerI::ClientSessionManagerI(const shared_ptr& factory) : _factory(factory) {} std::optional -ClientSessionManagerI::create(string user, std::optional ctl, const Ice::Current&) +ClientSessionManagerI::create(string user, std::optional ctl, const Ice::Current& current) { - return _factory->createGlacier2Session(std::move(user), std::move(ctl)); + return _factory->createGlacier2Session(std::move(user), std::move(ctl), current.con); } ClientSSLSessionManagerI::ClientSSLSessionManagerI(const shared_ptr& factory) : _factory(factory) @@ -340,7 +342,7 @@ std::optional ClientSSLSessionManagerI::create( Glacier2::SSLInfo info, std::optional ctl, - const Ice::Current&) + const Ice::Current& current) { string userDN; if (!info.certs.empty()) // TODO: Require userDN? @@ -360,5 +362,5 @@ ClientSSLSessionManagerI::create( } } - return _factory->createGlacier2Session(userDN, ctl); + return _factory->createGlacier2Session(userDN, ctl, current.con); } diff --git a/cpp/src/IceGrid/SessionI.h b/cpp/src/IceGrid/SessionI.h index 059af079736..749885579bf 100644 --- a/cpp/src/IceGrid/SessionI.h +++ b/cpp/src/IceGrid/SessionI.h @@ -24,8 +24,8 @@ namespace IceGrid public: virtual ~BaseSessionI() = default; - // Return value is never used. Just throws ONE when the session is destroyed. - std::chrono::steady_clock::time_point timestamp() const; + // Return value is never used. Just returns nullopt when the session is destroyed. + std::optional timestamp() const noexcept; void shutdown(); std::optional getGlacier2IdentitySet(); @@ -99,8 +99,10 @@ namespace IceGrid const IceInternal::TimerPtr&, const std::shared_ptr&); - Glacier2::SessionPrx - createGlacier2Session(const std::string&, const std::optional&); + Glacier2::SessionPrx createGlacier2Session( + const std::string& sessionId, + const std::optional& ctl, + const Ice::ConnectionPtr& con); std::shared_ptr createSessionServant(const std::string&);