Skip to content

PHPLIB-442: Ensure change stream resume token is updated after resume #621

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 21, 2019

Conversation

jmikola
Copy link
Member

@jmikola jmikola commented Jun 19, 2019

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

This extracts common logic in next() and rewind() to a new method, which is now also called by resume() after it rewinds the internal iterator.

@jmikola jmikola requested a review from kvwalker June 19, 2019 19:52
* only reference to any implicit session created therein. */
if ((string) $this->getCursorId() === '0') {
$this->resumeCallable = null;
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic previously happened after the steps below, but I believe the old ordering was incorrect. Consider:

  • If the iterator was not valid after calling next()/rewind(), we skipped the condition to set $hasAdvanced, optionally increment $key, and call extractResumeToken(). This cleanup check would always happen.
  • If the iterator was valid after calling next()/rewind(), we might potentially never reach this point if extractResumeToken() were to throw an exception.

This is basic cleanup that we'd always like to happen, so it now happens first.

* remaining results (up to and including the invalidate event) will
* have been received in the last response. Therefore, we can unset the
* resumeCallable. This will free any reference to Watch as well as the
* only reference to any implicit session created therein. */
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment was also revised a bit to better explain why we do this. I shared some thoughts on this in Slack and @p-mongo chimed in with a sanity check.

/* Triggering a consecutive failure will allow us to test whether the
* resume token was properly updated after the last resume. If the
* resume token updated, the next result will be {_id: 4}; otherwise,
* we'll see {_id: 3} returned again. */
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is basically a manual prose version of the spec test for PHPLIB-416 and DRIVERS-620, which is currently skipped in the master branch (see: here).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to check, this failed before the onIteration() change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. If I remove the call to onIteration(false) from resume(), testResumeTokenIsUpdatedAfterResuming fails and we encounter the error with duplicate events being returned as described in PHPLIB-442.

@jmikola
Copy link
Member Author

jmikola commented Jun 19, 2019

Note: the v1.4 branch does not have a replica set environment in Travis CI, so the new test is not executed. Here are the results of running the test suite locally against a 4.0.9 replica set:

$ vendor/bin/phpunit
PHPUnit 6.5.14 by Sebastian Bergmann and contributors.

.............................................................   61 / 2187 (  2%)
.............................................................  122 / 2187 (  5%)
.............................................................  183 / 2187 (  8%)
.............................RR..............................  244 / 2187 ( 11%)
.............................................................  305 / 2187 ( 13%)
.............................................................  366 / 2187 ( 16%)
.............................................................  427 / 2187 ( 19%)
.............................................................  488 / 2187 ( 22%)
.............................................................  549 / 2187 ( 25%)
.............................................................  610 / 2187 ( 27%)
..............................SSSS...........................  671 / 2187 ( 30%)
.............................................................  732 / 2187 ( 33%)
.............................................................  793 / 2187 ( 36%)
.............................................................  854 / 2187 ( 39%)
.............................................................  915 / 2187 ( 41%)
.............................................................  976 / 2187 ( 44%)
............................................................. 1037 / 2187 ( 47%)
............................................................. 1098 / 2187 ( 50%)
............................................................. 1159 / 2187 ( 52%)
............................................................. 1220 / 2187 ( 55%)
............................................................. 1281 / 2187 ( 58%)
............................................................. 1342 / 2187 ( 61%)
............................................................. 1403 / 2187 ( 64%)
............................................................. 1464 / 2187 ( 66%)
............................................................. 1525 / 2187 ( 69%)
............................................................. 1586 / 2187 ( 72%)
............................................................. 1647 / 2187 ( 75%)
............................................................. 1708 / 2187 ( 78%)
............................................................. 1769 / 2187 ( 80%)
............................................................. 1830 / 2187 ( 83%)
............................................................. 1891 / 2187 ( 86%)
............................................................. 1952 / 2187 ( 89%)
............................................................. 2013 / 2187 ( 92%)
............................................................. 2074 / 2187 ( 94%)
............................................................. 2135 / 2187 ( 97%)
....................................................          2187 / 2187 (100%)

Time: 24.72 seconds, Memory: 44.00MB

There were 2 risky tests:

1) MongoDB\Tests\DocumentationExamplesTest::testTransactions_intro_example_1
This test did not perform any assertions

2) MongoDB\Tests\DocumentationExamplesTest::testTransactions_retry_example_3
This test did not perform any assertions

OK, but incomplete, skipped, or risky tests!
Tests: 2187, Assertions: 6400, Skipped: 4, Risky: 2.

jmikola added 2 commits June 19, 2019 16:07
This extracts common logic in next() and rewind() to a new method, which is now also called by resume() after it rewinds the internal iterator.
Copy link
Contributor

@kvwalker kvwalker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good!

@jmikola jmikola merged commit 2c1cdb5 into mongodb:v1.4 Jun 21, 2019
jmikola added a commit that referenced this pull request Jun 21, 2019
@jmikola jmikola deleted the 1.4-phplib-442 branch June 21, 2019 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants