-
Notifications
You must be signed in to change notification settings - Fork 79
chore: Bump driver to latest #1206
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
Conversation
b0b150d
to
f7884e8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just fyi, I usually also bumped other driver dependencies along because they release on the same day (i.e. mongodb-client-encryption 2.0.0, no huge impact for us but it unblocks development on M1), but we can also do that independently
(Also, in terms of kerberos work, ideally they’ll release their library again soon with the bugfix for MONGOSH-1059 – not something this has to wait for, but a bugfix that we’d ideally want before the new connection form is fully release I think)
Oh, didn't thought about the other ones, totally makes sense to update them all. Thanks for the reminder! |
Was just bumping mongodb-client-encryption myself, but let's keep it in this pr then :) tested on M1 and works well! |
#0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1 0x00007f9f9e53e859 in __GI_abort () at abort.c:79
#2 0x00007f9f9e5a93ee in __libc_message (action=action@entry=do_abort, fmt=fmt@entry=0x7f9f9e6d3285 "%s\n") at ../sysdeps/posix/libc_fatal.c:155
#3 0x00007f9f9e5b147c in malloc_printerr (str=str@entry=0x7f9f9e6d51e0 "munmap_chunk(): invalid pointer") at malloc.c:5347
#4 0x00007f9f9e5b16cc in munmap_chunk (p=<optimized out>) at malloc.c:2830
#5 0x00000000017a3622 in std::_Function_handler<void (node_kerberos::KerberosWorker*), node_kerberos::KerberosClient::Step(Napi::CallbackInfo const&)::{lambda(std::function<void (std::function<void (node_kerberos::KerberosWorker*)>)>)#1}::operator()(std::function<void (std::function<void (node_kerberos::KerberosWorker*)>)>) const::{lambda(node_kerberos::KerberosWorker*)#1}>::_M_invoke(std::_Any_data const&, node_kerberos::KerberosWorker*&&) ()
#6 0x000000000179c1fa in node_kerberos::KerberosWorker::OnOK() ()
#7 0x00000000017971de in Napi::AsyncWorker::OnAsyncWorkComplete(napi_env__*, napi_status, void*) ()
#8 0x0000000000a73dd9 in node::ThreadPoolWork::ScheduleWork()::{lambda(uv_work_s*, int)#2}::_FUN(uv_work_s*, int) ()
#9 0x000000000141ccbd in uv__work_done (handle=0x7ffda89e9d20) at ../deps/uv/src/threadpool.c:311
#10 0x0000000001421456 in uv__async_io (loop=0x7ffda89e9c70, w=<optimized out>, events=<optimized out>) at ../deps/uv/src/unix/async.c:163
#11 0x00000000014338c4 in uv__io_poll (loop=loop@entry=0x7ffda89e9c70, timeout=<optimized out>) at ../deps/uv/src/unix/epoll.c:374
#12 0x0000000001421d88 in uv_run (loop=0x7ffda89e9c70, mode=UV_RUN_DEFAULT) at ../deps/uv/src/unix/core.c:389
#13 0x0000000001776216 in RunNodeInstance(node::MultiIsolatePlatform*, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&) ()
#14 0x00000000009f64c1 in main () |
and I definitely did not mean to close this, sorry :) |
You mean it's something in the encryption package? Should we exclude the bump for this package from this PR then or keep it open and investigate / fix / update the version before merging? |
"version": "0.1.1", | ||
"resolved": "https://registry.npmjs.org/noop-logger/-/noop-logger-0.1.1.tgz", | ||
"integrity": "sha1-lKKxYzxPExdVMAfYlm/Q6EG2pMI=", | ||
"version": "4.3.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The e2e test failures are happening because of this – the node-addon-api Error
class constructor isn’t ABI-compatible between 4.3.0 and previous versions due to this addition: https://github.com/nodejs/node-addon-api/blame/2c88a7ec4c6d14a27182f5de6a5166faa9e86567/napi.h#L1723
Since in the compiled binary multiple node-addon-api instances from different addons end up being linked together, that can end up being problematic (in this case, the kerberos addon ends up using the newer, incompatible node-addon-api from mongodb-client-encryption for creating Error objects, but uses inline code from the older node-addon-api for destroying them). I’ve opened a PR for node-addon-api to undo this incompatibility, since there was never a good reason for adding it to begin with, but obviously that won’t be released automatically either.
I’m not sure what the best approach here is. mongodb-js/kerberos#142 will update the dependency for kerberos in the same way.
Ideally, we probably need to find a way to avoid these conflicts altogether. For now, maybe we could try to standardize on node-addon-api 4.3.0 across the board? #1205 updates macos-export-certificate-and-key/win-export-certificate-and-key to versions that also use that node-addon-api version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thanks for the detailed explanation! Aligning them sounds like a good idea, from what I can see in addition to these certificate packages we would also need to bump os-dns-native
, right? (here's an npm ls node-addon-api
from mongosh):
mongosh: mongosh@0.0.0-dev.0 /Users/sergey.petushkov/Projects/MongoDB/mongosh/packages/mongosh
mongosh: └─┬ @mongosh/cli-repl@0.0.0-dev.0 -> ./../cli-repl
mongosh: ├─┬ @mongosh/logging@0.0.0-dev.0 -> ./../logging
mongosh: │ └─┬ @mongodb-js/devtools-connect@1.1.3
mongosh: │ └─┬ os-dns-native@1.1.2
mongosh: │ └── node-addon-api@3.2.1
mongosh: ├─┬ @mongosh/service-provider-core@0.0.0-dev.0 -> ./../service-provider-core
mongosh: │ └─┬ mongodb-client-encryption@2.0.0
mongosh: │ └── node-addon-api@4.3.0
mongosh: ├─┬ @mongosh/service-provider-server@0.0.0-dev.0 -> ./../service-provider-server
mongosh: │ ├─┬ @mongodb-js/devtools-connect@1.1.3
mongosh: │ │ └─┬ os-dns-native@1.1.2
mongosh: │ │ └── node-addon-api@3.2.1
mongosh: │ ├─┬ kerberos@2.0.0-beta.0
mongosh: │ │ └── node-addon-api@4.2.0
mongosh: │ └─┬ mongodb-client-encryption@2.0.0
mongosh: │ └── node-addon-api@4.3.0
mongosh: ├─┬ @mongosh/types@0.0.0-dev.0 -> ./../types
mongosh: │ └─┬ @mongodb-js/devtools-connect@1.1.3
mongosh: │ └─┬ os-dns-native@1.1.2
mongosh: │ └── node-addon-api@3.2.1
mongosh: └─┬ macos-export-certificate-and-key@1.0.2
mongosh: └── node-addon-api@1.7.2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think we'll need to bump the node-addon-api version there before this (but I would expect the upgrade to Just Work 🙂)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like it indeed "just worked" 😄 mongodb-js/os-dns-native#4
Driver release notes: https://github.com/mongodb/node-mongodb-native/releases/tag/v4.4.0
(lerna bootstrap never succeeded for me locally failing with various TS errors, but I'm guessing I don't know a proper way to run it in this monorepo or something is messed up with my setup, so I'm opening a PR anyway to see if it will pass in CI)I think I finally figured it out, let's see