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(NODE-6583): update to fixed BSON version #4338

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

nbbeeken
Copy link
Contributor

Description

What is changing?

Is there new documentation needed for these changes?

What is the motivation for this change?

Release Highlight

Fill in title or leave empty for no highlight

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@nbbeeken nbbeeken changed the title [WIP] chore(NODE-6552): fix performance test issue fix(NODE-6583): update to fixed BSON version Nov 25, 2024
Copy link
Contributor Author

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

This is waiting on mongodb/js-bson#740 to be merged and released.

Comment on lines +397 to +401
reject(
new MongoNetworkTimeoutError(
`Socket '${connectEvent}' timed out after ${(performance.now() - start) | 0}ms (connectTimeoutMS: ${connectTimeoutMS})`
)
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't mind putting these all back, but if they were helpful to debug once I think they would be again.

}
// Finally, now treat the resulting duplex stream as the
// socket over which we send and receive wire protocol messages:
return await makeSocket({ ...options, existingSocket, proxyHost: undefined });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We probably want to keep this change. We should only convert the errors from Socks to our network errors, makeSocket will already throw the correct type, or if it doesn't we wouldn't know in the SOCKs case because we always convert.

Some "null access" error becomes a retryable network error only under socks 😅 could be confusing!

Comment on lines +9 to +14
/**
* The path to the MongoDB Node.js driver.
* This MUST be set to the directory the driver is installed in
* NOT the file "lib/index.js" that is the driver's export.
*/
const MONGODB_DRIVER_PATH = (() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also happy to pull out all this revision/version stuff. I think I've adequately made it fail nicely when the files/env vars don't exist. Again, it provided value once so it could again.

@@ -67,7 +129,7 @@ function initCollection() {

function dropCollection() {
return this.collection.drop().catch(e => {
if (e.code !== MONGODB_ERROR_CODES.NamespaceNotFound) {
if (e.code !== 26 /* NamespaceNotFound */) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't public API so testing arbitrary driver versions made this difficult when only importing at the package level.

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.

1 participant