Python 4542 - Improved sessions API#2712
Conversation
| if session is not None: | ||
| session._process_response(reply) | ||
|
|
||
| def _get_bound_session(self) -> Optional[AsyncClientSession]: |
There was a problem hiding this comment.
This is encapsulated in a separate utility function because cursor operations call _ensure_session directly, while most other database operations call _tmp_session instead. Separating out this check keeps the clarity of the current behavioral differences with minimal code duplication.
NoahStapp
requested review from
ShaneHarvey,
aclark4life and
caseyclements
and removed request for
caseyclements
|
|
||
| - Added the :meth:`~pymongo.asynchronous.client_session.AsyncClientSession.bind` and :meth:`~pymongo.client_session.ClientSession.bind` methods | ||
| that allow users to bind a session to all database operations within the scope of a context manager instead of having to explicitly pass the session to each individual operation. | ||
| See <PLACEHOLDER> for examples and more information. |
There was a problem hiding this comment.
<PLACEHOLDER> should be the MongoDB docs ?
There was a problem hiding this comment.
Yeah once we have examples and such added to a page I'll update these spots.
| self._attached_to_cursor = False | ||
| # Should we leave the session alive when the cursor is closed? | ||
| self._leave_alive = False | ||
| # Is this session bound to a scope? |
There was a problem hiding this comment.
Nit: I had to look up what "scope" means in this context so maybe "# Is this session bound to a context manager scope?"
| return bound_session.session | ||
| else: | ||
| raise InvalidOperation( | ||
| "Only the client that created the bound session can perform operations within its context block. See <PLACEHOLDER> for more information." |
There was a problem hiding this comment.
Another <PLACEHOLDER> here … assuming that maybe these come out after merge?
| def _ensure_session(self, session: Optional[ClientSession] = None) -> Optional[ClientSession]: | ||
| """If provided session is None, lend a temporary session.""" | ||
| """If provided session and bound session are None, lend a temporary session.""" | ||
| session = session or self._get_bound_session() |
There was a problem hiding this comment.
I don't understand what's going on here before the changes. How does:
if session:
return session
lend a temporary session?
There was a problem hiding this comment.
We call _ensure_session(session) in a bunch of places where session is an actual explicit session if the user passed one to the operation. The former if session: return session check is to return that explicit session if it exists instead of lending a temporary one.
| return bound_session.session | ||
| else: | ||
| raise InvalidOperation( | ||
| "Only the client that created the bound session can perform operations within its context block. See <PLACEHOLDER> for more information." |
There was a problem hiding this comment.
Third <PLACEHOLDER> maybe synchro'd
There was a problem hiding this comment.
Pull request overview
Adds a “bound session” mechanism to PyMongo’s explicit sessions so users can scope a ClientSession/AsyncClientSession to all operations within a context block (without passing session=... to every call).
Changes:
- Introduces
ClientSession.bind()/AsyncClientSession.bind()backed by aContextVar, and updatesMongoClient/AsyncMongoClientto use a bound session when no session is explicitly provided. - Expands sync/async session tests to cover bound-session behavior and nested binding.
- Updates changelog and synchro/unasync replacement mapping for the new internal bound-session helper type.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/synchro.py | Adds unasync replacement mapping for the new bound-session helper type. |
| pymongo/synchronous/client_session.py | Adds ContextVar-based bound-session support and ClientSession.bind(). |
| pymongo/synchronous/mongo_client.py | Uses bound session in _ensure_session/_tmp_session and adds _get_bound_session(). |
| pymongo/asynchronous/client_session.py | Adds ContextVar-based bound-session support and AsyncClientSession.bind(). |
| pymongo/asynchronous/mongo_client.py | Uses bound session in _ensure_session/_tmp_session and adds _get_bound_session(). |
| test/test_session.py | Adds coverage for bound sessions and nested binding (sync). |
| test/asynchronous/test_session.py | Adds coverage for bound sessions and nested binding (async). |
| doc/changelog.rst | Documents the new API in the changelog (currently with placeholders). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def bind(self) -> ClientSession: | ||
| self._bound = True | ||
| return self | ||
|
|
There was a problem hiding this comment.
bind() mutates session state (self._bound = True) and returns self, which changes the meaning of using the session itself as a context manager. In particular, with client.start_session().bind() as s: (or calling s.bind() earlier and later doing with s:) will no longer end the session on exit, leaking server sessions and breaking the long-standing contract documented in this module. Consider making bind() return a dedicated context manager (separate object) that only sets/resets the bound-session ContextVar, while keeping ClientSession.__enter__/__exit__ semantics unchanged (always ending the session).
| def bind(self) -> ClientSession: | |
| self._bound = True | |
| return self | |
| class _BindContext(ContextManager["ClientSession"]): | |
| """Context manager used by ClientSession.bind(). | |
| Temporarily marks the session as bound so that __enter__/__exit__ | |
| manage the bound-session ContextVar, and then restores the previous | |
| bound state on exit. | |
| """ | |
| def __init__(self, session: "ClientSession") -> None: | |
| self._session = session | |
| self._prev_bound = session._bound | |
| def __enter__(self) -> "ClientSession": | |
| # Mark the session as bound for the duration of this context and | |
| # reuse ClientSession.__enter__ to set the ContextVar. | |
| self._session._bound = True | |
| return self._session.__enter__() | |
| def __exit__( | |
| self, | |
| exc_type: Optional[Type[BaseException]], | |
| exc_val: Optional[BaseException], | |
| exc_tb: Optional["TracebackType"], | |
| ) -> None: | |
| try: | |
| # Delegate to ClientSession.__exit__ to reset the ContextVar | |
| # without ending the session when bound. | |
| self._session.__exit__(exc_type, exc_val, exc_tb) | |
| finally: | |
| # Restore the previous bound state. | |
| self._session._bound = self._prev_bound | |
| def bind(self) -> ContextManager["ClientSession"]: | |
| """Return a context manager that binds this session to the current context. | |
| Using ``with session.bind():`` will temporarily bind the session to the | |
| bound-session ContextVar without permanently changing the session's | |
| behavior when used as a context manager itself. | |
| """ | |
| return self._BindContext(self) |
| self.client_id = client_id | ||
|
|
||
|
|
||
| class AsyncBoundSessionContext: |
There was a problem hiding this comment.
Should this class be private?
There was a problem hiding this comment.
Can you explain how the changes in 5edd9a0 address this concern (assuming they do) ?
There was a problem hiding this comment.
AsyncBoundSessionContext is now private since users aren't intended to use it outside of a context manager.
pymongo/asynchronous/mongo_client.py
Outdated
| def _get_bound_session(self) -> Optional[AsyncClientSession]: | ||
| bound_session = _SESSION.get() | ||
| if bound_session: | ||
| if bound_session.client_id == id(self): |
There was a problem hiding this comment.
Could we replace this with session.client is self? Then we can get rid of the _AsyncBoundClientSession class altogether.
There was a problem hiding this comment.
Same question about 5edd9a0, this issue is addressed in that commit correct (and if so how) ?
There was a problem hiding this comment.
Shane's suggestion was implemented and the _AsyncBoundClientSession removed.
|
Added the new # Create session outside of a context manager and use bind() multiple times
session = client.start_session()
with session.bind():
...
with session.bind():
...
# User must explicitly end session
session.end_session()
# Create session and bind it in a single context manager
# Does NOT end the session when s goes out of scope, user needs to call s.end_session() explicitly
with client.start_session().bind() as s:
...
s.end_session()
# Create session and bind it in a single context manager
# Ends the session when s goes out of scope
with client.start_session().bind(end_session=True) as s:
...
# Create session in a context manager and call bind() multiple times
# Ends the session when s goes out of scope
with client.start_session() as s:
with s.bind():
...
...
with s.bind():
... |
|
Is |
It may be better since, if they do the same thing, I prefer the former to the latter. It would also never occur to me to do: |
|
Chaining doesn't work because bind() doesn't return a session object. It returns a private context manager. So |
| _SESSION: ContextVar[Optional[ClientSession]] = ContextVar("SESSION", default=None) | ||
|
|
||
|
|
||
| class BoundSessionContext: |
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Here's what I mean: with client.start_session().bind() as s:
client.t.t.insert_one({}, session=s) # This will fail because "s" is not a session Another problem is that if with client.start_session().bind():
# ...
# session is leaked here.This is problematic because it can lead to hitting the session cache size limit on the server. It also points to a design issue. New APIs should be easy to use correctly and arguably more importantly hard to use incorrectly. So far we've added an api with the first property but not the second. Noah and I discussed in person and decided to make with client.start_session().bind(end_session=False): # This mistakenly leaks the session
# ...
with client.start_session().bind(): # This is correct.
# ...
with client.start_session() as s, s.bind(): # This is correct.
# ...
with client.start_session() as s, s.bind(end_session=False): # This is correct.
# ...
# This is correct.
with client.start_session() as s:
with s.bind(end_session=False):
# ...
with s.bind(end_session=False):
# ... |
| raise InvalidOperation("Cannot use ended session") | ||
|
|
||
| def bind(self, end_session: bool = True) -> _AsyncBoundSessionContext: | ||
| """Bind this session so it is implicitly passed to all database operations within the returned context. |
There was a problem hiding this comment.
Could you add a short docs explain here? We still do that right?
There was a problem hiding this comment.
Like a .. code-block:: python example
[PYTHON-4542]
Changes in this PR
Add
ClientSession.bind()(and its async counterpartAsyncClientSession.bind()) for a better user experience when using explicit sessions.Test Plan
Modified
TestSession._test_ops()to include explicit bound sessions. Added a new test to verify correct nested behavior of multipleClientSession.bind()calls and a new test to verify correctness of theend_sessionparameter ofbind().Checklist
Checklist for Author
Checklist for Reviewer