SERVER-38810 Simplify correct usage of locking in Session/SessionCatalog.

This patch introduces two new types for gaining access to sessions in
the session catalog, based on the context of the access. It leaves a
third way to access sessions, which is via
OperationContextSession::get().

The new types are ObservableSession and SessionToKill. With this
change, a thread may access a session in one of three ways:

(1) by binding the session to an OperationContext for regular use, by
instantiation of OperationContextSession or
MongoDOperationContextSession, as before.

(2) In the callback passed to SessionCatalog::scanSessions, in which
case the callback is passed an ObservableSession that represents
looking at a Session while the SessionCatalog mutex is locked and
while the bound OperationContext's client mutex is held, if the
session is currently checked out via methods (1) or (3).

(3) By calling SessionCatalog::checkOutSessionForKill, which returns a
KillableSession. This is used for cleaning a session up after it is
marked for kill.

This patch eliminates Session::_mutex, which is no longer required, as
the SessionCatalog::_mutex and the Client provide all necessary mutual
exclusion for the Session type itself.

1 files changed, 147 insertions, 48 deletions

diff --git a/src/mongo/db/session_catalog.h b/src/mongo/db/session_catalog.h
index 1ead5cad736..c22c96ff8ba 100644
--- a/src/mongo/db/session_catalog.h
+++ b/src/mongo/db/session_catalog.h
@@ -34,7 +34,9 @@
 #include <vector>
 
 #include "mongo/base/disallow_copying.h"
+#include "mongo/db/client.h"
 #include "mongo/db/logical_session_id.h"
+#include "mongo/db/operation_context.h"
 #include "mongo/db/session.h"
 #include "mongo/db/session_killer.h"
 #include "mongo/stdx/condition_variable.h"
@@ -44,10 +46,7 @@
 
 namespace mongo {
 
-class OperationContext;
-class ScopedSession;
-class ScopedCheckedOutSession;
-class ServiceContext;
+class ObservableSession;
 
 /**
  * Keeps track of the transaction runtime state for every active session on this instance.
@@ -55,10 +54,21 @@ class ServiceContext;
 class SessionCatalog {
     MONGO_DISALLOW_COPYING(SessionCatalog);
 
-    friend class ScopedSession;
-    friend class ScopedCheckedOutSession;
+    friend class ObservableSession;
+    friend class OperationContextSession;
 
 public:
+    class ScopedCheckedOutSession;
+    class SessionToKill;
+
+    struct KillToken {
+        KillToken(LogicalSessionId lsid) : lsidToKill(std::move(lsid)) {}
+        KillToken(KillToken&&) = default;
+        KillToken& operator=(KillToken&&) = default;
+
+        LogicalSessionId lsidToKill;
+    };
+
     SessionCatalog() = default;
     ~SessionCatalog();
 
@@ -75,28 +85,10 @@ public:
     void reset_forTest();
 
     /**
-     * Potentially blocking call, which either creates a brand new session object (if one doesn't
-     * exist) or "checks-out" the existing one (if it is not currently in use or marked for kill).
-     *
-     * The 'opCtx'-only variant uses the session information stored on the operation context and the
-     * variant, which has the 'lsid' parameter checks-out that session id. Neither of these methods
-     * can be called with an already checked-out session.
-     *
-     * Checking out a session puts it in the 'checked out' state and all subsequent calls to
-     * checkout will block until it is checked back in. This happens when the returned object goes
-     * out of scope.
-     *
-     * Throws exception on errors.
-     */
-    ScopedCheckedOutSession checkOutSession(OperationContext* opCtx);
-    ScopedCheckedOutSession checkOutSession(OperationContext* opCtx, const LogicalSessionId& lsid);
-
-    /**
-     * See the description of 'Session::kill' for more information on the session kill usage
-     * pattern.
+     * See the description of 'ObservableSession::kill' for more information on the session kill
+     * usage pattern.
      */
-    ScopedCheckedOutSession checkOutSessionForKill(OperationContext* opCtx,
-                                                   Session::KillToken killToken);
+    SessionToKill checkOutSessionForKill(OperationContext* opCtx, KillToken killToken);
 
     /**
      * Iterates through the SessionCatalog under the SessionCatalog mutex and applies 'workerFn' to
@@ -109,7 +101,7 @@ public:
      *
      * TODO SERVER-33850: Take Matcher out of the SessionKiller namespace.
      */
-    using ScanSessionsCallbackFn = stdx::function<void(WithLock, Session*)>;
+    using ScanSessionsCallbackFn = stdx::function<void(const ObservableSession&)>;
     void scanSessions(const SessionKiller::Matcher& matcher,
                       const ScanSessionsCallbackFn& workerFn);
 
@@ -117,7 +109,7 @@ public:
      * Shortcut to invoke 'kill' on the specified session under the SessionCatalog mutex. Throws a
      * NoSuchSession exception if the session doesn't exist.
      */
-    Session::KillToken killSession(const LogicalSessionId& lsid);
+    KillToken killSession(const LogicalSessionId& lsid);
 
 private:
     struct SessionRuntimeInfo {
@@ -132,6 +124,8 @@ private:
         stdx::condition_variable availableCondVar;
     };
 
+    ScopedCheckedOutSession _checkOutSession(OperationContext* opCtx);
+
     /**
      * May release and re-acquire it zero or more times before returning. The returned
      * 'SessionRuntimeInfo' is guaranteed to be linked on the catalog's _txnTable as long as the
@@ -144,7 +138,7 @@ private:
      * Makes a session, previously checked out through 'checkoutSession', available again.
      */
     void _releaseSession(std::shared_ptr<SessionRuntimeInfo> sri,
-                         boost::optional<Session::KillToken> killToken);
+                         boost::optional<KillToken> killToken);
 
     stdx::mutex _mutex;
 
@@ -153,19 +147,20 @@ private:
 };
 
 /**
- * Scoped object representing a checked-out session. See comments for the 'checkoutSession' method
- * for more information on its behaviour.
+ * Scoped object representing a checked-out session. This type is an implementation detail
+ * of the SessionCatalog.
  */
-class ScopedCheckedOutSession {
-    MONGO_DISALLOW_COPYING(ScopedCheckedOutSession);
-
-    friend ScopedCheckedOutSession SessionCatalog::checkOutSession(OperationContext*,
-                                                                   const LogicalSessionId&);
-    friend ScopedCheckedOutSession SessionCatalog::checkOutSessionForKill(OperationContext*,
-                                                                          Session::KillToken);
-
+class SessionCatalog::ScopedCheckedOutSession {
 public:
+    ScopedCheckedOutSession(SessionCatalog& catalog,
+                            std::shared_ptr<SessionCatalog::SessionRuntimeInfo> sri,
+                            boost::optional<SessionCatalog::KillToken> killToken)
+        : _catalog(catalog), _sri(std::move(sri)), _killToken(std::move(killToken)) {}
+
     ScopedCheckedOutSession(ScopedCheckedOutSession&&) = default;
+    ScopedCheckedOutSession& operator=(ScopedCheckedOutSession&&) = delete;
+    ScopedCheckedOutSession(const ScopedCheckedOutSession&) = delete;
+    ScopedCheckedOutSession& operator=(ScopedCheckedOutSession&) = delete;
 
     ~ScopedCheckedOutSession() {
         if (_sri) {
@@ -190,19 +185,117 @@ public:
     }
 
 private:
-    ScopedCheckedOutSession(SessionCatalog& catalog,
-                            std::shared_ptr<SessionCatalog::SessionRuntimeInfo> sri,
-                            boost::optional<Session::KillToken> killToken)
-        : _catalog(catalog), _sri(std::move(sri)), _killToken(std::move(killToken)) {}
-
     // The owning session catalog into which the session should be checked back
     SessionCatalog& _catalog;
 
     std::shared_ptr<SessionCatalog::SessionRuntimeInfo> _sri;
+    boost::optional<SessionCatalog::KillToken> _killToken;
+};
+
+/**
+ * RAII type returned by SessionCatalog::checkOutSessionForKill.
+ *
+ * After calling kill() on an ObservableSession, let that ObservableSession go out
+ * of scope and in a context outside of SessionCatalog::scanSessions, call checkOutSessionForKill
+ * to get an instance of this type. Then, while holding that instance, perform any cleanup
+ * you need to perform on a session as part of killing it. More details in the description of
+ * ObservableSession::kill, below.
+ */
+class SessionCatalog::SessionToKill {
+public:
+    SessionToKill(ScopedCheckedOutSession&& scos) : _scos(std::move(scos)) {}
+    Session* get() const {
+        return _scos.get();
+    }
+
+    const LogicalSessionId& getSessionId() const {
+        return get()->getSessionId();
+    }
+    OperationContext* currentOperation_forTest() const {
+        return get()->currentOperation_forTest();
+    }
 
-    // Only set if the session was obtained though checkOutSessionForKill
-    boost::optional<Session::KillToken> _killToken;
+private:
+    ScopedCheckedOutSession _scos;
 };
+using SessionToKill = SessionCatalog::SessionToKill;
+
+/**
+ * This type represents access to a session inside of a scanSessions loop.
+ * If you have one of these, you're in a scanSessions callback context, and so
+ * have locked the whole catalog and, if the observed session is bound to an operation context,
+ * you hold that operation context's client's mutex, as well.
+ */
+class ObservableSession {
+public:
+    ObservableSession(const ObservableSession&) = delete;
+    ObservableSession(ObservableSession&&) = delete;
+    ObservableSession& operator=(const ObservableSession&) = delete;
+    ObservableSession& operator=(ObservableSession&&) = delete;
+
+    /**
+     * The logical session id that this object represents.
+     */
+    const LogicalSessionId& getSessionId() const {
+        return _session->_sessionId;
+    }
+
+    /**
+     * Returns a pointer to the current operation running on this Session, or nullptr if there is no
+     * operation currently running on this Session.
+     */
+    OperationContext* currentOperation() const;
+
+    /**
+     * Increments the number of "killers" for this session and returns a 'kill token' to to be
+     * passed later on to 'checkOutSessionForKill' method of the SessionCatalog in order to permit
+     * the caller to execute any kill cleanup tasks. This token is later on passed to
+     * '_markNotKilled' in order to decrement the number of "killers".
+     *
+     * Marking session as killed is an internal property only that will cause any further calls to
+     * 'checkOutSession' to block until 'checkOutSessionForKill' is called the same number of times
+     * as 'kill' was called and the returned scoped object destroyed.
+     *
+     * If the first killer finds the session checked-out, this method will also interrupt the
+     * operation context which has it checked-out.
+     */
+    SessionCatalog::KillToken kill(ErrorCodes::Error reason = ErrorCodes::Interrupted) const;
+
+    /**
+     * Returns a pointer to the Session itself.
+     */
+    Session* get() const {
+        return _session;
+    }
+
+private:
+    friend class SessionCatalog;
+
+    static stdx::unique_lock<Client> _lockClientForSession(WithLock, Session* session) {
+        if (const auto opCtx = session->_checkoutOpCtx) {
+            return stdx::unique_lock<Client>{*opCtx->getClient()};
+        }
+        return {};
+    }
+
+    ObservableSession(WithLock wl, Session& session)
+        : _session(&session), _clientLock(_lockClientForSession(std::move(wl), _session)) {}
+
+    /**
+     * Returns whether 'kill' has been called on this session.
+     */
+    bool _killed() const;
+
+    /**
+     * Used by the session catalog when checking a session back in after a call to 'kill'. See the
+     * comments for 'kill for more details.
+     */
+    void _markNotKilled(WithLock sessionCatalogLock, SessionCatalog::KillToken killToken);
+
+    Session* _session;
+    stdx::unique_lock<Client> _clientLock;
+};
+
 
 /**
  * Scoped object, which checks out the session specified in the passed operation context and stores
@@ -213,11 +306,17 @@ class OperationContextSession {
     MONGO_DISALLOW_COPYING(OperationContextSession);
 
 public:
+    /**
+     * Acquires the session with id opCtx->getLogicalSessionId().  Because a session can only be
+     * checked out by one user at a time, construction of OperationContextSession can block waiting
+     * for the desired session to be checked in by another user.
+     */
     OperationContextSession(OperationContext* opCtx);
     ~OperationContextSession();
 
     /**
-     * Returns the session checked out in the constructor.
+     * Returns the session currently checked out by "opCtx", or nullptr if the opCtx has no
+     * checked out session.
      */
     static Session* get(OperationContext* opCtx);