-
Notifications
You must be signed in to change notification settings - Fork 266
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
Conversation
* only reference to any implicit session created therein. */ | ||
if ((string) $this->getCursorId() === '0') { | ||
$this->resumeCallable = null; | ||
} |
There was a problem hiding this comment.
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 callextractResumeToken()
. This cleanup check would always happen. - If the iterator was valid after calling
next()
/rewind()
, we might potentially never reach this point ifextractResumeToken()
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. */ |
There was a problem hiding this comment.
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. */ |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Note: the
|
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good!
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.