fix(ChangeStream): hasNext/next should work after resume #2333
Conversation
| @@ -4,7 +4,7 @@ language: node_js | |||
| branches: | |||
| only: | |||
| - master | |||
| - 3.6 | |||
| - next | |||
There was a problem hiding this comment.
Accidentally left this change in the previous merge from 3.6, so I reverted it here. But really we should just delete .travis.yml since we don't use Travis and therefore don't maintain this file.
| @@ -786,7 +793,7 @@ describe('Change Streams', function() { | |||
| } | |||
| }); | |||
|
|
|||
| it('Should return MongoNetworkError after first retry attempt fails using promises', { | |||
| it.skip('Should return MongoNetworkError after first retry attempt fails using promises', { | |||
There was a problem hiding this comment.
These twoShould return MongoNetworkError after first retry attempt fails tests started timing out now that .hasNext resumes from errors. They just keep resuming from the network errors; none of the resume attempts fail. I'm guessing these tests are quite old and it might make more sense to remove them than to try to fix them.
| let server; | ||
| let client; | ||
| let changeStream; | ||
| const oldLabelTest = label => |
There was a problem hiding this comment.
Should I just remove these tests for the old labels?
| const nextP = changeStream.next(); | ||
|
|
||
| return changeStream.close().then(() => nextP); | ||
| const nextP = () => changeStream.next(); |
There was a problem hiding this comment.
I had to change nextP into a function to prevent the .next from returning successfully before the close on the next line.
| expect(err).to.not.exist; | ||
| }); | ||
| }, | ||
| 250 |
There was a problem hiding this comment.
Added this small delay because triggering the resumable error immediately after waitForStarted causes weird behavior, and isn't particularly realistic.
| @@ -2800,7 +2815,7 @@ describe('Change Streams', function() { | |||
| case 2: | |||
| // only events after this point are relevant to this test | |||
| events = []; | |||
| triggerResumableError(changeStream, () => events.push('error')); | |||
| triggerResumableError(changeStream, () => events.push('error'), 0); | |||
There was a problem hiding this comment.
However, in this case the resumable error needs to be instant because of how the test is constructed; it has to trigger before the next change event is processed.
|
I pulled a bunch of changes from this PR that weren't relevant into #2343. I'll open a new PR to replace this one with just the relevant changes, once that one is merged. |
|
Superseded by #2360. |
Description
NODE-2548
What changed?
getCursormethod to provide fresh cursor after resuming from an errornext/hasNextto usegetCursorprocessNewChangemaybePromiseworkrecreateCursorto reduce duplication inwaitForTopology/createChangeStreamCursorlogiccreateChangeStreamCursortocreateCursorChangeStream.closeto usemaybePromiseAre there any files to ignore?