PHPC-1152, PHPC-1161: Emulate implicit sessions for command cursors #803
Conversation
jmikola
force-pushed
the
1.4-phpc-1152
branch
2 times, most recently
from
2c001f4 to
393d274
Compare
jmikola
force-pushed
the
1.4-phpc-1152
branch
3 times, most recently
from
d8b9fd2 to
04c50b5
Compare
jmikola
changed the title
jmikola
changed the title
jmikola
changed the title
| int result; | ||
| bool result = false; | ||
| bool free_reply = false; | ||
| bool free_zsession = false; |
There was a problem hiding this comment.
Before you merge this into master, you should run clang-format on this.
There was a problem hiding this comment.
This is in the v1.4 branch, which isn't formatted. I intend to run it through clang-format after (or during) the merge to master. Does that work?
php_phongo.c
Outdated
| /* If an explicit session was not provided, attempt to create an implicit | ||
| * client session (ignoring any errors). | ||
| * | ||
| * TODO: Ensure that this doesn't conflict with any commands users may run |
There was a problem hiding this comment.
How are you going to "ensure" this? Right now, it just looks like a comment that we're going to forget about :-)
There was a problem hiding this comment.
Indeed. This is something I wanted to check manually but running through a few cases in Exceptions to sending the session ID to the server on all commands. In particular, I'd like to test:
isMasterexecuteWriteCommand()with an unacknowledged write concern
I'm also curious about killCursors. The spec says that an LSID is optional, but testing in PHPC-1152 demonstrated that killCursors wasn't always using the LSID of the command from which the cursor originated. I want to confirm that the exceptions for mismatched LSIDs only applied to getMore and that killCursors truly doesn't care. If that's not the case, I think the spec language may need some corrections.
There was a problem hiding this comment.
@kvwalker answered the killCursors question in mongodb/mongo-php-library#528 (comment).
There was a problem hiding this comment.
isMaster isn't a problem.
Unacknowledged write concerns is a legitimate issue, and may also affect libmongoc. I've opened PHPC-1163 to track that.
There was a problem hiding this comment.
LGTM. I tested this locally with PHPLIB-324 and verified that resumeCallable is unset after the cursor is invalidated, which I tested using DropDatabase before calling $changeStream->next().
jmikola
force-pushed
the
1.4-phpc-1152
branch
2 times, most recently
from
c038283 to
5e8c1be
Compare
This corrects the function signatures to return a boolean indicating success instead of an integer (which was cast from a boolean). Additionally, it restructures phongo_execute_command() to use a cleanup label, in anticipation of implicit session changes.
This ensures that phongo_execute_command creates an implicit session (if supported and not explicit session was provided). In turn, this ensures that any command cursor shares the same session as its originating command. By creating a Session object, we can ensure that the implicit session is destroyed during garbage collection when the last reference is removed.
This should have done in cc90304.
https://jira.mongodb.org/browse/PHPC-1152
https://jira.mongodb.org/browse/PHPC-1161
Tests for this patch depend on #811, although I've included a temporary commit with the patch (sans the full patch and tests in #811) to give this PR's own tests a chance at passing.