Skip to content
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

feat: upgrade mongodb -> 6.8.0, handle throwing error on closed cursor in Mongoose #14813

Merged
merged 10 commits into from
Aug 21, 2024

Conversation

vkarpov15
Copy link
Collaborator

@vkarpov15 vkarpov15 commented Aug 17, 2024

Summary

We haven't upgraded to MongoDB Node driver 6.8 because we have a test failure due to a minor breaking change in the driver: cursor.next() no longer errors out after calling cursor.close(). We reported an issue in the MongoDB Node driver JIRA here: https://jira.mongodb.org/browse/NODE-6255 and the issue is in progress, but I'm thinking that 1) better for Mongoose to enforce this behavior if our tests rely on it, 2) we've already been blocked from upgrading to mongodb@6.8 for 6 weeks.

Examples

@vkarpov15 vkarpov15 added this to the 8.6 milestone Aug 17, 2024
@hasezoey hasezoey changed the title feat: upgrade mongodb -> 6.8.0, handle throwing error on closed curso… feat: upgrade mongodb -> 6.8.0, handle throwing error on closed cursor in Mongoose Aug 18, 2024
Copy link
Collaborator

@hasezoey hasezoey left a comment

Choose a reason for hiding this comment

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

Code looks OK, though if we should go with the current solution and not get a answer in the JIRA, then it should likely be noted that the error (class & message) is different.

also the replicaset tests seem to fail consistently even after a re-run:

   3 failing
  1) Model
       bug fixes
         3.6 features
           watch()
             "after each" hook in "watch()":
     done() called multiple times in hook <Model bug fixes 3.6 features watch() "after each" hook in "watch()"> of file /home/runner/work/mongoose/mongoose/test/model.test.js; in addition, done() received error: MongoAPIError: ChangeStream is closed
    at ChangeStream._processChange (/home/runner/work/mongoose/mongoose/node_modules/mongodb/lib/change_stream.js:309:19)
    at ReadableCursorStream.<anonymous> (/home/runner/work/mongoose/mongoose/node_modules/mongodb/lib/change_stream.js:287:46)
    at ReadableCursorStream.emit (node:events:513:28)
    at addChunk (node:internal/streams/readable:315:12)
    at readableAddChunk (node:internal/streams/readable:289:9)
    at ReadableCursorStream.Readable.push (node:internal/streams/readable:228:10)
    at /home/runner/work/mongoose/mongoose/node_modules/mongodb/lib/cursor/abstract_cursor.js:649:26
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (node:internal/process/task_queues:96:5) {
  uncaught: true,
  [Symbol(errorLabels)]: Set(0) {}
}
  Error: done() called multiple times in hook <Model bug fixes 3.6 features watch() "after each" hook in "watch()"> of file /home/runner/work/mongoose/mongoose/test/model.test.js; in addition, done() received error: MongoAPIError: ChangeStream is closed
      at ChangeStream._processChange (node_modules/mongodb/lib/change_stream.js:309:19)
      at ReadableCursorStream.<anonymous> (node_modules/mongodb/lib/change_stream.js:287:46)
      at ReadableCursorStream.emit (node:events:513:28)
      at addChunk (node:internal/streams/readable:315:12)
      at readableAddChunk (node:internal/streams/readable:289:9)
      at ReadableCursorStream.Readable.push (node:internal/streams/readable:228:10)
      at /home/runner/work/mongoose/mongoose/node_modules/mongodb/lib/cursor/abstract_cursor.js:649:26
      at runMicrotasks (<anonymous>)
      at processTicksAndRejections (node:internal/process/task_queues:96:5) {
    uncaught: true,
    [Symbol(errorLabels)]: Set(0) {}
  }
      at process.emit (node:events:513:28)
      at process._fatalException (node:internal/process/execution:149:25)

  2) Model
       bug fixes
         3.6 features
           watch()
             watch() close() closes the stream (gh-7022):
     Uncaught MongoAPIError: ChangeStream is closed
      at ChangeStream._processChange (node_modules/mongodb/lib/change_stream.js:309:19)
      at ReadableCursorStream.<anonymous> (node_modules/mongodb/lib/change_stream.js:287:46)
      at ReadableCursorStream.emit (node:events:513:28)
      at addChunk (node:internal/streams/readable:315:12)
      at readableAddChunk (node:internal/streams/readable:289:9)
      at ReadableCursorStream.Readable.push (node:internal/streams/readable:228:10)
      at /home/runner/work/mongoose/mongoose/node_modules/mongodb/lib/cursor/abstract_cursor.js:649:26
      at runMicrotasks (<anonymous>)
      at processTicksAndRejections (node:internal/process/task_queues:96:5)

  3) Model
       bug fixes
         3.6 features
           watch()
             watch() close() closes the stream (gh-7022):
     Error: done() called multiple times in test <Model bug fixes 3.6 features watch() watch() close() closes the stream (gh-7022)> of file /home/runner/work/mongoose/mongoose/test/model.test.js
  

Copy link
Collaborator

@hasezoey hasezoey left a comment

Choose a reason for hiding this comment

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

LGTM, though same comment as before about the JIRA:

though if we should go with the current solution and not get a answer in the JIRA, then it should likely be noted that the error (class & message) is different.

@vkarpov15
Copy link
Collaborator Author

We will make a note in the changelog that the error class and message changed

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