PHPLIB-342: Change streams should use the same session when resuming #528
Conversation
src/ChangeStream.php
Outdated
| // resume and we can therefore unset the resumeCallable, which will | ||
| // free any reference to Watch. This will also free the only | ||
| // reference to an implicit session, since any such reference | ||
| // belongs to Watch. |
There was a problem hiding this comment.
/* Feel free to
* use multi-line
* comments. */
src/ChangeStream.php
Outdated
| // resume and we can therefore unset the resumeCallable, which will | ||
| // free any reference to Watch. This will also free the only | ||
| // reference to an implicit session, since any such reference | ||
| // belongs to Watch. |
There was a problem hiding this comment.
I propose pointing to the earlier extended comment for next() rather than reproducing the full explanation:
// As with next(), free the callable once we know it will never be used
| ); | ||
|
|
||
| $changeStream->rewind(); | ||
| $this->killChangeStreamCursor($changeStream); |
There was a problem hiding this comment.
Please test this against a replica set with auth if possible. That should highlight whether killChangeStreamCursor() will need to also take the same implicit session created within the Watch class.
There was a problem hiding this comment.
After testing with auth, it looks like killChangeStreamCursor() does not need to take the same implicit session created within the Watch class.
There was a problem hiding this comment.
Excellent. That answers my question in mongodb/mongo-php-driver#803 (comment).
| }, | ||
| function($changeStream) use (&$originalSession) { | ||
| if (isset($changeStream->aggregate)) { | ||
| $originalSession = $changeStream->lsid; |
There was a problem hiding this comment.
AFAIK lsid may include other fields, so you may want to only capture lsid->id (and/or possibly cast it to a hex string as I'm doing in the PHPC tests:
bin2hex((string) $command->lsid->id);
This makes output easier to digest if comparisons fail, since you're no longer dealing with raw binary strings and MongoDB\BSON\Binary objects.
|
|
||
| $this->assertSame($expectedCommands, $commands); | ||
|
|
||
| foreach ($sessionAfterResume as $session) { |
src/ChangeStream.php
Outdated
| * free any reference to Watch. This will also free the only | ||
| * reference to an implicit session, since any such reference | ||
| * belongs to Watch. */ | ||
| if ($this->getCursorId() === 0) { |
There was a problem hiding this comment.
getCursorId() returns a MongoDB\Driver\CursorId, so this will never evaluate to true. You may need to cast the CursorId to a string and compare with '0'.
kvwalker
changed the title
|
Let's rebase on v1.3 tomorrow before you merge this. |
https://jira.mongodb.org/browse/PHPLIB-342
I marked this [WIP] because I'll update
WatchFunctionalTestto verify that thelsids of all aggregates and getMores are the same after PHPC-1152 is merged.