PHPLIB-441: Increment ChangeStream key consistently when resuming #626

jmikola · 2019-06-21T22:15:26Z

https://jira.mongodb.org/browse/PHPLIB-441

jmikola · 2019-06-21T22:18:01Z

src/ChangeStream.php

-        /* Increment the key if the iteration event was a call to next() and we
-         * have already advanced past the first result. */
-        if ($isNext && $this->hasAdvanced) {
+        if ($incrementKey) {


I contemplated renaming this to $incrementKeyIfValid, but adding a comment above the early return for !$this->valid() seemed preferable.

I think the comment makes this clear.

jmikola · 2019-06-21T22:25:09Z

src/ChangeStream.php

+         * LogicException for attempting to rewind an advanced cursor. In the
+         * latter case, the exception would have bypassed the catch block for
+         * resuming. */
+        $this->onIteration($this->hasAdvanced);


I'd appreciate a sanity check on this. While coming up with the current implementation in this PR, I ran the test suite with a hard assertion of $this->hasAdvanced === false placed before the call to resume() in rewind()'s catch block, and it was never triggered.

I'm not sure I follow this part:

If it were true, then the previous call to IteratorIterator::rewind() would either have been a NOP and succeeded because the cursor was still at its first position

How can $hasAdvanced be true if the cursor was still at its first position?

My point here was that "if it were true" cannot happen in practice. Either the rewind() succeeds as a NOP and doesn't throw, so we never call resume() in the first place, or it throws a LogicException, which we don't catch (and also don't call resume()). I'll reword this a bit to better explain that, as I feel I only conveyed that the LogicException case is when we never reach this point.

How can $hasAdvanced be true if the cursor was still at its first position?

If this was a more general question (apart from resume() being invoked), I can address that. When a cursor is first created (basic or tailable alike), it starts before the first element. valid() is false and there is no current element. The initial call to rewind() or next() is responsible for advancing to the first position (if any), updating valid(), and optionally populating the current element.

We use $hasAdvanced here to track if we've advanced before the initial pre-iteration state. If it's more helpful, I suppose we can rename this to $hasIterationStarted. Let me know if you think that'd make more sense.

Another edge case is that it's kosher to call rewind() multiple times in sequence. And since $key is initialized to 0, we should never increment the key after a successful call to rewind(), since, unlike next() or resuming, rewind() itself never moves past the first element.

Oh I see, thanks for the explanation! All of this makes sense.

I suppose we can rename this to $hasIterationStarted.

Ah, I forgot that I actually added a doc block to $hasAdvanced to explain exactly what it implies (here). I think that's sufficient, as $hasIterated might also sound confusing since calls to rewind() and next() can still leave it false if the change stream doesn't come across a change event. I'll stick with the shorter name and just revise this comment block to be more clear.

Note: this is a slight edge case where rewind() could be called immediately after resuming without encountering a LogicException from the driver. Since resuming creates a fresh cursor and rewinds it, a user could technically call ChangeStream::rewind() after a resume and not see an error. This is very trivial and may not be worth fixing, but I opened PHPLIB-448 to track it.

jmikola · 2019-06-21T22:26:01Z

tests/Operation/WatchFunctionalTest.php

@@ -225,6 +226,62 @@ private function assertStartAtOperationTime(TimestampInterface $expectedOperatio
        $this->assertEquals($expectedOperationTime, $command->pipeline[0]->{'$changeStream'}->startAtOperationTime);
    }

+    public function testRewindMultipleTimesWithResults()


These two methods aren't related to resuming, so I included them in their own commit. This is the rewind() counterpart to the existing testNextAdvancesKey test method.

jmikola · 2019-06-21T22:28:14Z

tests/Operation/WatchFunctionalTest.php

@@ -322,20 +379,67 @@ public function testNoChangeAfterResumeBeforeInsert()
        $this->assertMatchesDocument($expectedResult, $changeStream->current());
    }

-    public function testResumeTokenIsUpdatedAfterResuming()
+    public function testResumeMultipleTimesInSuccession()


This method was introduced in #621 for PHPLIB-442 earlier this week, so I didn't feel too bad about renaming it. Refactoring it to also check for key increments (in addition to the existing result assertions) seemed preferable to duplicating the entire test to only check key increments.

I also added some additional resume cases, which weren't in the original test.

jmikola · 2019-06-21T22:29:44Z

tests/Operation/WatchFunctionalTest.php

@@ -689,6 +795,8 @@ public function testNextAdvancesKey()
        $this->insertDocument(['x' => 1]);
        $this->insertDocument(['x' => 2]);

+        /* Note: we intentionally do not start iteration with rewind() to ensure
+         * that next() behaves identically when called without rewind(). */


I happened to read through this test while writing the multiple rewind tests and thought this could use some clarity. This change is also in its own commit, since it doesn't pertain to resuming.

kvwalker · 2019-06-25T15:10:16Z

src/ChangeStream.php

-        /* Increment the key if the iteration event was a call to next() and we
-         * have already advanced past the first result. */
-        if ($isNext && $this->hasAdvanced) {
+        if ($incrementKey) {


I think the comment makes this clear.

kvwalker · 2019-06-25T15:13:48Z

src/ChangeStream.php

+         * LogicException for attempting to rewind an advanced cursor. In the
+         * latter case, the exception would have bypassed the catch block for
+         * resuming. */
+        $this->onIteration($this->hasAdvanced);


I'm not sure I follow this part:

If it were true, then the previous call to IteratorIterator::rewind() would either have been a NOP and succeeded because the cursor was still at its first position

How can $hasAdvanced be true if the cursor was still at its first position?

kvwalker · 2019-06-25T15:14:40Z

tests/Operation/WatchFunctionalTest.php


        $operation = new Watch($this->manager, $this->getDatabaseName(), $this->getCollectionName(), [], $this->defaultOptions);
        $changeStream = $operation->execute($this->getPrimaryServer());

+        /* Killing the cursor when there are no results will tests that neither


nit: tests should be `test

kvwalker

lgtm!

jmikola · 2019-06-25T19:51:32Z

Test failure fixed in #628.

jmikola added 2 commits June 21, 2019 17:01

Test behavior for multiple calls to ChangeStream::rewind()

adbea29

Explain why rewind() is not used in testNextAdvancesKey()

909121f

jmikola requested a review from kvwalker June 21, 2019 22:15

jmikola changed the base branch from master to v1.4 June 21, 2019 22:15

jmikola force-pushed the 1.4-phplib-441 branch from a962767 to dea9c32 Compare June 21, 2019 22:22

jmikola commented Jun 21, 2019

View reviewed changes

kvwalker reviewed Jun 25, 2019

View reviewed changes

kvwalker approved these changes Jun 25, 2019

View reviewed changes

PHPLIB-441: Increment ChangeStream key consistently when resuming

fdf87ad

jmikola force-pushed the 1.4-phplib-441 branch from dea9c32 to fdf87ad Compare June 25, 2019 18:27

jmikola merged commit fdf87ad into mongodb:v1.4 Jun 25, 2019

jmikola added a commit that referenced this pull request Jun 25, 2019

Merge pull request #626

b19328a

PHPLIB-441: Increment ChangeStream key consistently when resuming #626

PHPLIB-441: Increment ChangeStream key consistently when resuming #626

Uh oh!

Conversation

jmikola commented Jun 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmikola Jun 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kvwalker left a comment

Choose a reason for hiding this comment

Uh oh!

jmikola commented Jun 25, 2019

Uh oh!

Uh oh!

jmikola Jun 25, 2019 •

edited

Loading