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

fix(cursor): throw error in ChangeStream constructor if changeStreamThunk() throws a sync error #14846

Merged
merged 2 commits into from
Sep 3, 2024

Conversation

vkarpov15
Copy link
Collaborator

Summary

An unintended consequence of #14813 using promises rather than event emitters is that sync errors in changeStreamThunk() aren't possible to handle. Model.watch().on('error') doesn't work because the constructor throws an error before the on('error') call happens.

This PR makes ChangeStream() constructor throw an error if changeStreamThunk() throws a synchronous error. The code is a little clunky, but at least avoids silently ignoring sync errors in changeStreamThunk(). This code is hard to test because changeStreamThunk() doesn't throw in Mongoose in general.

Examples

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 good to me.

Though some of the replica-set runs (both push & pull_request) had failed with: (which i assume has nothing todo with this PR in particular)

   1) transactions
       distinct (gh-8006):
     MongoServerError: Given transaction number 4 does not match any in-progress transactions. The active transaction number is 3
      at Connection.sendCommand (node_modules/mongodb/lib/cmap/connection.js:290:27)
      at runMicrotasks (<anonymous>)
      at processTicksAndRejections (node:internal/process/task_queues:96:5)
      at async Connection.command (node_modules/mongodb/lib/cmap/connection.js:313:26)
      at async Server.command (node_modules/mongodb/lib/sdam/server.js:167:29)
      at async InsertOneOperation.executeCommand (node_modules/mongodb/lib/operations/command.js:73:16)
      at async InsertOneOperation.execute (node_modules/mongodb/lib/operations/insert.js:37:16)
      at async InsertOneOperation.execute (node_modules/mongodb/lib/operations/insert.js:46:21)
      at async executeOperation (node_modules/mongodb/lib/operations/execute_operation.js:136:16)
      at async Collection.insertOne (node_modules/mongodb/lib/collection.js:154:16)

@vkarpov15
Copy link
Collaborator Author

@hasezoey correct, looks like a flakey test

@vkarpov15 vkarpov15 added this to the 8.6.1 milestone Sep 3, 2024
@vkarpov15 vkarpov15 merged commit 94e1237 into master Sep 3, 2024
46 checks passed
@hasezoey hasezoey deleted the vkarpov15/changestream-sync-error branch September 3, 2024 20:31
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