fix(ChangeStream): should resume from errors when iterating

[emadum]

Description

Introduced getCursor method to safely provide a change stream cursor
for next/hasNext across recoveries from resumable errors.

NODE-2548

What changed?

• next and hasNext now use getCursor
• processError was extracted from processNewChange
• added withCursor shared test helper

Are there any files to ignore?

[mbroadst]

Nice refactor for processError! This is looking really great, I've just got some questions and a few suggestions.

[mbroadst]

I think this might be useful as a more generalized helper withCursor, which we could alias to withChangeStream if that helps readability at all. Also, I think hiding a call to withClient in here might not be ideal since its sort of a side effect.

Finally, nitpick but testName seems to really be like collectionName?

[emadum]

Waiting on #2362 to see how to best address this.

[mbroadst]

I didn't ultimately find the withDb or withCollection helpers helped very much over there, but cursors are much more like clients, in that they have explicit close methods that sometimes need to be called.

[emadum]

I added a withCursor helper; it's a general-purpose wrapper around withClient. Is this roughly what you had in mind?

function withCursor(getCursor, callback) {
  return withClient((client, done) => {
    getCursor(client, (err, cursor) => {
      if (err) return done(err);
      callback(cursor, () => cursor.close(done));
    });
  });
}

[mbroadst]

I think of withX as a means of conveying the lifetime of a resource. So my thought with withCursor is that it would be like withClient, but not use withClient:

function withCursor(cursor, body) {
   body(cursor, (err, result) => {
     cursor.close(); // always close the cursor, this obviously would need to be async and handle errors etc
     
      callback(...);
   });
}

[mbroadst]

it may not be worth the effort considering we have this.defer?

const cursor = coll.find();
this.defer(() => cursor.close());
cursor.toArray(...);

could work fine, but you might forget to put the defer in between creating the cursor and iterating it

[mbroadst]

I didn't ultimately find the withDb or withCollection helpers helped very much over there, but cursors are much more like clients, in that they have explicit close methods that sometimes need to be called.

[reggi]

LGTM

[mbroadst]

LGTM!

[mbroadst]

Just a thought, and not one I wanted to block this review on:

This helper doesn't account for tests which return a promise, which is a similar discussion to ones we've had previously in withClient and friends. I wonder if what we eventually want is some helper like withCloseable which is internally used by withClient, withMonitoredClient, withCursor and maybe even something like withSession (though this is an existing helper in the public API). Food for thought

[emadum]

Agreed it would be nice for withMonitoredClient and withCursor to support promises as well as callbacks, the way withClient does. I think refactoring to use withCloseable would be a nice reduction of duplicated logic but I don't see how it would add much utility. Perhaps it's something we can work into a future PR where we're trying to write promise-based tests that need monitored clients or cursors?