PYTHON-3587 Do not perform server selection to determine sessions support #1491

NoahStapp · 2024-01-30T21:34:45Z

No description provided.

NoahStapp · 2024-01-30T21:35:52Z

pymongo/client_session.py

-        # Although the Driver Sessions Spec says we only clear stale sessions
-        # in return_server_session, PyMongo can't take a lock when returning
-        # sessions from a __del__ method (like in Cursor.__die), so it can't
-        # clear stale sessions there. In case many sessions were returned via
-        # __del__, check for stale sessions here too.
-        self._clear_stale(session_timeout_minutes)
-
+    def get_server_session(self) -> _ServerSession:
        # The most recently used sessions are on the left.
        while self:
-            s = self.popleft()
-            if not s.timed_out(session_timeout_minutes):
-                return s


This change is necessary because we no longer have access to session_timeout_minutes at the time of session creation. Is it feasible to move this timeout check to the moment of session application instead, where we have the underlying connection and access to the timeout?

Let's keep this method the same but delay calling it until ClientSession._materialize() is called.

ShaneHarvey · 2024-01-31T00:17:13Z

pymongo/client_session.py

-        # Although the Driver Sessions Spec says we only clear stale sessions
-        # in return_server_session, PyMongo can't take a lock when returning
-        # sessions from a __del__ method (like in Cursor.__die), so it can't
-        # clear stale sessions there. In case many sessions were returned via
-        # __del__, check for stale sessions here too.
-        self._clear_stale(session_timeout_minutes)
-
+    def get_server_session(self) -> _ServerSession:
        # The most recently used sessions are on the left.
        while self:
-            s = self.popleft()
-            if not s.timed_out(session_timeout_minutes):
-                return s


Let's keep this method the same but delay calling it until ClientSession._materialize() is called.

ShaneHarvey · 2024-01-31T00:18:39Z

pymongo/mongo_client.py

        if implicit:
-            self._topology._check_implicit_session_support()
            server_session: Union[_EmptyServerSession, _ServerSession] = _EmptyServerSession()
        else:
            server_session = self._get_server_session()


Let's have both implicit and explicit sessions start with _EmptyServerSession(). That way we can check logicalSessionTimeoutMinutes on first use in _materialize().

ShaneHarvey · 2024-01-31T00:19:47Z

pymongo/server_description.py

@@ -243,8 +243,7 @@ def is_server_type_known(self) -> bool:
    def retryable_writes_supported(self) -> bool:
        """Checks if this server supports retryable writes."""
        return (
-            self._ls_timeout_minutes is not None


Should we remove _ls_timeout_minutes and all code for it?

I believe so, there's no more use for it.

Actually, according to the spec https://github.com/mongodb/specifications/blob/master/source/sessions/driver-sessions.rst, we still need to store the lowest logicalSessionTimeoutMinutes value in the ToplogyDescription.

I see, so we still need to track it so we can use the lowest value for the server session cache expiration.

ShaneHarvey · 2024-01-31T00:20:00Z

pymongo/topology.py

@@ -609,8 +609,7 @@ def _check_session_support(self) -> float:
    def get_server_session(self) -> _ServerSession:
        """Start or resume a server session, or raise ConfigurationError."""
        with self._lock:
-            session_timeout = self._check_session_support()


Can we remove _check_session_support?

ShaneHarvey · 2024-01-31T00:21:22Z

test/test_encryption.py

+    MONGOCRYPTD_PORT = 27020
+
+    @classmethod
+    @client_context.require_sessions


This test is for servers that don't support sessions, but require_sessions does the opposite. Can we remove this?

ShaneHarvey · 2024-01-31T00:24:17Z

test/test_encryption.py

+
+    def setUp(self) -> None:
+        self.listener = OvertCommandListener()
+        self.listener.reset()


Let's remove this reset() call since the listener was just created.

ShaneHarvey · 2024-01-31T00:25:36Z

test/test_encryption.py

+    def test_explicit_session_errors_when_unsupported(self):
+        self.listener.reset()
+        with self.mongocryptd_client.start_session() as s:
+            with self.assertRaises(ConfigurationError):


Let's use assertRaisesRegex to assert the error message too.

ShaneHarvey · 2024-01-31T00:26:45Z

test/test_encryption.py

+                self.mongocryptd_client.db.test.find_one(session=s)
+            with self.assertRaises(ConfigurationError):
+                self.mongocryptd_client.db.test.insert_one({"x": 1}, session=s)
+        self.mongocryptd_client.close()


instead of closing at the end of each test I suggest using self.addCleanup(self.mongocryptd_client.close) right after creating the client in setup.

ShaneHarvey · 2024-01-31T00:27:06Z

test/test_encryption.py

@@ -22,6 +22,7 @@
 import socket
 import socketserver
 import ssl
+import subprocess


WIP Fix spawn args

ShaneHarvey · 2024-01-31T22:48:38Z

pymongo/client_session.py

@@ -965,10 +965,12 @@ def _txn_read_preference(self) -> Optional[_ServerMode]:
            return self._transaction.opts.read_preference
        return None

-    def _materialize(self) -> None:
+    def _materialize(self, logical_session_timeout_minutes: float) -> None:


float -> int.

The original code has session_timeout_minutes as a float, is that an unfixed mistake?

Yeah that was a mistake. It's int everywhere else.

ShaneHarvey · 2024-01-31T22:48:58Z

pymongo/client_session.py

@@ -1097,7 +1104,9 @@ def pop_all(self) -> list[_ServerSession]:
            ids.append(self.pop().session_id)
        return ids

-    def get_server_session(self, session_timeout_minutes: float) -> _ServerSession:
+    def get_server_session(
+        self, session_timeout_minutes: float, old: _EmptyServerSession


float -> int.

ShaneHarvey · 2024-01-31T22:53:53Z

pymongo/client_session.py

@@ -1111,7 +1120,7 @@ def get_server_session(self, session_timeout_minutes: float) -> _ServerSession:
            if not s.timed_out(session_timeout_minutes):
                return s

-        return _ServerSession(self.generation)
+        return _ServerSession(self.generation, old.session_id)


If the cache is empty we'll create a new session with the same id but if the cache isn't empty we'll return an existing session with a different id. What problem are we trying to solve here?

Hmm, good point if the cache is not empty. Our unified test runner expects that sessions have an id on creation, with several tests relying on that behavior. Adding a simple ping command after creation to populate the session id emits unexpected events that cause those same tests to fail.

Oh I see. I think the only approach is that accessing the session_id property needs to materialize the session too either without checking the lstm field or using the topologies lstm (either way without doing server selection).

Sorry, I don't quite understand what you mean. You're saying we need to have the session materialize when session_id is accessed, but without actually checking if sessions are supported?

Yes. The session support check would still happen later in _apply_to.

ShaneHarvey · 2024-01-31T22:56:01Z

pymongo/pool.py

@@ -870,6 +871,7 @@ def _hello(
        self.max_message_size = hello.max_message_size
        self.max_write_batch_size = hello.max_write_batch_size
        self.supports_sessions = hello.logical_session_timeout_minutes is not None
+        self.logical_session_timeout_minutes = hello.logical_session_timeout_minutes or 0.0


or 0 since logical_session_timeout_minutes is an int.

ShaneHarvey · 2024-02-01T21:38:41Z

test/test_session.py

@@ -116,16 +116,12 @@ def _test_ops(self, client, *ops):

        for f, args, kw in ops:
            with client.start_session() as s:
-                last_use = s._server_session.last_use
-                start = time.monotonic()
-                self.assertLessEqual(last_use, start)


Do we have any other tests for last_use? Are we loosing coverage here?

No, we can fix the issue by materializing the session before accessing last_use.

pymongo/client_session.py

ShaneHarvey · 2024-02-01T22:04:07Z

pymongo/mongo_client.py

-                if self._retryable and not supports_session:
+                sessions_supported = (
+                    self._retryable
+                    and self._server.description.retryable_writes_supported


Can we remove this line?

And just check conn.supports_sessions instead?

We can remove two of the variables in the check, the remaining two are required for tests to pass.

pymongo/client_session.py

ShaneHarvey · 2024-02-02T00:40:00Z

pymongo/mongo_client.py

            with self._client._checkout(self._server, self._session) as conn:
                max_wire_version = conn.max_wire_version
-                if self._retryable and not supports_session:
+                sessions_supported = (
+                    self._server.description.retryable_writes_supported and conn.supports_sessions


Is self._session never None anymore?

Added it back for safety.

ShaneHarvey · 2024-02-02T00:42:11Z

test/test_session.py

@@ -245,10 +245,12 @@ def test_pool_lifo(self):
        b.end_session()

        s = self.client.start_session()
+        self.client.admin.command("ping", session=s)


Can we remove these pings now that s.session_id materializes the session?

No, this test requires the sessions actually enter the pool, which materializing itself does not do. The test fails without these pings, sadly.

Can you explain? It appears this test is only checking session_id.

The test checks that session a and b are both in the session pool after they are ended, with b at the top due to LIFO ordering. We expect the next session created to be b, but since session creation no longer interacts with the pool unless a timeout is present, materializing the new session instead of using a ping results in a new session entirely.

Oh that's a bug in get_server_session. After it's fixed we should remove the pings here.

ShaneHarvey · 2024-02-02T00:44:18Z

test/test_session.py

@@ -274,6 +276,8 @@ def test_end_sessions(self):
        client = rs_or_single_client(event_listeners=[listener])
        # Start many sessions.
        sessions = [client.start_session() for _ in range(_MAX_END_SESSIONS + 1)]
+        for s in sessions:
+            client.admin.command("ping", session=s)


This is gonna make the test take a long time. Can we instead just do s.session_id?

ShaneHarvey · 2024-02-02T01:15:53Z

pymongo/client_session.py

+            while self:
+                s = self.popleft()
+                if not s.timed_out(session_timeout_minutes):
+                    return s


We still want to acquire a session from the pool here even if the timeout is None.

ShaneHarvey · 2024-02-02T01:16:17Z

test/test_session.py

@@ -245,10 +245,12 @@ def test_pool_lifo(self):
        b.end_session()

        s = self.client.start_session()
+        self.client.admin.command("ping", session=s)


Oh that's a bug in get_server_session. After it's fixed we should remove the pings here.

ShaneHarvey

Could you schedule the mockupdb tests in EVG? It would also be good to run the motor test suite after merging this to see if anything breaks there.

ShaneHarvey · 2024-02-02T18:36:57Z

pymongo/client_session.py

+        if session_timeout_minutes is None:
+            return False
+        else:
+            idle_seconds = time.monotonic() - self.last_use


nit: since the if-statement returns we don't need the else. The benefit is that:

code is easier to read with less indentation

there will be less code churn in the PR and in the git history.

What do you think?

NoahStapp · 2024-02-02T20:02:23Z

Could you schedule the mockupdb tests in EVG? It would also be good to run the motor test suite after merging this to see if anything breaks there.

mockupdb tests timed out on test_rsghost: https://spruce.mongodb.com/version/65bd38bbc9ec443a931ddce7/tasks?sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC

ShaneHarvey · 2024-02-02T20:55:00Z

Looks like the test gets stuck attempting to run endSessions when closing the client in test_rsghost:

 [2024/02/02 11:19:49.085] Thread 0x00007f64de62f740 (most recent call first):
 [2024/02/02 11:19:49.085]   File "/data/mci/1adae7ce73c33f49ebb23345c8812e53/src/pymongo/socket_checker.py", line 66 in select
 [2024/02/02 11:19:49.085]   File "/data/mci/1adae7ce73c33f49ebb23345c8812e53/src/pymongo/network.py", line 321 in wait_for_read
 [2024/02/02 11:19:49.085]   File "/data/mci/1adae7ce73c33f49ebb23345c8812e53/src/pymongo/network.py", line 340 in _receive_data_on_socket
 [2024/02/02 11:19:49.085]   File "/data/mci/1adae7ce73c33f49ebb23345c8812e53/src/pymongo/network.py", line 263 in receive_message
 [2024/02/02 11:19:49.085]   File "/data/mci/1adae7ce73c33f49ebb23345c8812e53/src/pymongo/network.py", line 186 in command
 [2024/02/02 11:19:49.085]   File "/data/mci/1adae7ce73c33f49ebb23345c8812e53/src/pymongo/pool.py", line 997 in command
 [2024/02/02 11:19:49.085]   File "/data/mci/1adae7ce73c33f49ebb23345c8812e53/src/pymongo/helpers.py", line 322 in inner
 [2024/02/02 11:19:49.085]   File "/data/mci/1adae7ce73c33f49ebb23345c8812e53/src/pymongo/mongo_client.py", line 1193 in _end_sessions
 [2024/02/02 11:19:49.085]   File "/data/mci/1adae7ce73c33f49ebb23345c8812e53/src/pymongo/mongo_client.py", line 1216 in close
 [2024/02/02 11:19:49.085]   File "/data/mci/1adae7ce73c33f49ebb23345c8812e53/src/pymongo/mongo_client.py", line 2112 in __exit__
 [2024/02/02 11:19:49.085]   File "/data/mci/1adae7ce73c33f49ebb23345c8812e53/src/test/mockupdb/test_rsghost.py", line 56 in test_rsghost

NoahStapp · 2024-02-02T20:59:56Z

Looks like the test gets stuck attempting to run endSessions when closing the client in test_rsghost:

 [2024/02/02 11:19:49.085] Thread 0x00007f64de62f740 (most recent call first):
 [2024/02/02 11:19:49.085]   File "/data/mci/1adae7ce73c33f49ebb23345c8812e53/src/pymongo/socket_checker.py", line 66 in select
 [2024/02/02 11:19:49.085]   File "/data/mci/1adae7ce73c33f49ebb23345c8812e53/src/pymongo/network.py", line 321 in wait_for_read
 [2024/02/02 11:19:49.085]   File "/data/mci/1adae7ce73c33f49ebb23345c8812e53/src/pymongo/network.py", line 340 in _receive_data_on_socket
 [2024/02/02 11:19:49.085]   File "/data/mci/1adae7ce73c33f49ebb23345c8812e53/src/pymongo/network.py", line 263 in receive_message
 [2024/02/02 11:19:49.085]   File "/data/mci/1adae7ce73c33f49ebb23345c8812e53/src/pymongo/network.py", line 186 in command
 [2024/02/02 11:19:49.085]   File "/data/mci/1adae7ce73c33f49ebb23345c8812e53/src/pymongo/pool.py", line 997 in command
 [2024/02/02 11:19:49.085]   File "/data/mci/1adae7ce73c33f49ebb23345c8812e53/src/pymongo/helpers.py", line 322 in inner
 [2024/02/02 11:19:49.085]   File "/data/mci/1adae7ce73c33f49ebb23345c8812e53/src/pymongo/mongo_client.py", line 1193 in _end_sessions
 [2024/02/02 11:19:49.085]   File "/data/mci/1adae7ce73c33f49ebb23345c8812e53/src/pymongo/mongo_client.py", line 1216 in close
 [2024/02/02 11:19:49.085]   File "/data/mci/1adae7ce73c33f49ebb23345c8812e53/src/pymongo/mongo_client.py", line 2112 in __exit__
 [2024/02/02 11:19:49.085]   File "/data/mci/1adae7ce73c33f49ebb23345c8812e53/src/test/mockupdb/test_rsghost.py", line 56 in test_rsghost

Yeah, while sending the endSessions commands back to the server.

ShaneHarvey · 2024-02-02T21:01:32Z

test_rsghost needs to set up an auto responder for endSessions. Although I wonder why the endSession was not needed before.

ShaneHarvey · 2024-02-02T21:09:39Z

Previously we never sent sessions to RSGhost servers but now we do. That could be a problem.

NoahStapp · 2024-02-02T21:15:45Z

Previously we never sent sessions to RSGhost servers but now we do. That could be a problem.

We do now because of the sessions changes? That shouldn't happen, should it?

NoahStapp · 2024-02-05T20:30:00Z

Passing mockupdb tests: https://spruce.mongodb.com/version/65c13ac13e8e86a4392b3080/tasks?sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC

ShaneHarvey · 2024-02-05T20:34:48Z

pymongo/pool.py

@@ -752,6 +752,7 @@ def __init__(
        self.active = False
        self.last_timeout = self.opts.socket_timeout
        self.connect_rtt = 0.0
+        self.logical_session_timeout_minutes: int = 0


Let's default this to None or avoid confusing the 0 and None cases. (It's technically possible to set logical_session_timeout_minutes=0.)

ShaneHarvey · 2024-02-05T20:35:04Z

pymongo/pool.py

+        self.supports_sessions = (
+            hello.logical_session_timeout_minutes is not None and hello.is_readable
+        )
+        self.logical_session_timeout_minutes = hello.logical_session_timeout_minutes or 0


Let's remove the or 0.

ShaneHarvey

LGTM!

NoahStapp requested a review from a team as a code owner January 30, 2024 21:34

NoahStapp requested review from caseyclements and removed request for a team January 30, 2024 21:34

NoahStapp commented Jan 30, 2024

View reviewed changes

NoahStapp requested a review from ShaneHarvey January 31, 2024 00:05

ShaneHarvey requested changes Jan 31, 2024

View reviewed changes

NoahStapp added 5 commits January 31, 2024 12:18

WIP

e68438f

WIP Fix spawn args

Fix spawn args

c855a4d

Require 4.2+ for tests

b8240ea

Skip session not supported tests unless pymongocrypt is present

f94ad1b

Fix tests to account for later session support checking

8edb2d3

NoahStapp force-pushed the PYTHON-3587 branch from f12ae28 to 8edb2d3 Compare January 31, 2024 20:18

NoahStapp requested a review from ShaneHarvey January 31, 2024 20:18

ShaneHarvey requested changes Jan 31, 2024

View reviewed changes

ShaneHarvey reviewed Jan 31, 2024

View reviewed changes

logical_session_timeout_minutes float -> int

589a2c8

NoahStapp requested a review from ShaneHarvey February 1, 2024 00:13

NoahStapp added 4 commits February 1, 2024 11:13

Materialize _EmptyServerSession when session_id is accessed

0a8f605

Cleanup

6144aef

Materialize empty sessions when getting transaction_id

2cf1f66

session.transaction_id -> session._transaction_id

66bcf90

ShaneHarvey requested changes Feb 1, 2024

View reviewed changes

ShaneHarvey reviewed Feb 1, 2024

View reviewed changes

pymongo/client_session.py Outdated Show resolved Hide resolved

Address review comments

2f9bb20

NoahStapp requested a review from ShaneHarvey February 2, 2024 00:22

ShaneHarvey reviewed Feb 2, 2024

View reviewed changes

Remove unneeded sesson pings

4d9b965

NoahStapp requested a review from ShaneHarvey February 2, 2024 00:57

ShaneHarvey requested changes Feb 2, 2024

View reviewed changes

Fix get_server_session

9b91b5c

NoahStapp requested a review from ShaneHarvey February 2, 2024 17:46

ShaneHarvey requested changes Feb 2, 2024

View reviewed changes

Remove unneeded else

0bbf30e

caseyclements approved these changes Feb 2, 2024

View reviewed changes

Use hello.is_readable in connection for session support

c2cee78

NoahStapp requested a review from ShaneHarvey February 5, 2024 20:29

ShaneHarvey requested changes Feb 5, 2024

View reviewed changes

Let self.logical_session_timeout_minutes default to None

ff38be2

NoahStapp requested a review from ShaneHarvey February 5, 2024 20:55

ShaneHarvey approved these changes Feb 5, 2024

View reviewed changes

NoahStapp merged commit 62c6d0f into mongodb:master Feb 5, 2024
68 of 69 checks passed

PYTHON-3587 Do not perform server selection to determine sessions support #1491

PYTHON-3587 Do not perform server selection to determine sessions support #1491

Conversation

NoahStapp commented Jan 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaneHarvey Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaneHarvey Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaneHarvey Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaneHarvey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NoahStapp commented Feb 2, 2024

ShaneHarvey commented Feb 2, 2024 • edited Loading

NoahStapp commented Feb 2, 2024

ShaneHarvey commented Feb 2, 2024

ShaneHarvey commented Feb 2, 2024

NoahStapp commented Feb 2, 2024

NoahStapp commented Feb 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaneHarvey left a comment

Choose a reason for hiding this comment

ShaneHarvey Jan 31, 2024 •

edited

Loading

ShaneHarvey Jan 31, 2024 •

edited

Loading

ShaneHarvey Feb 2, 2024 •

edited

Loading

ShaneHarvey commented Feb 2, 2024 •

edited

Loading