feat: upgrade to mongodb@6#83
Conversation
robertsLando
left a comment
robertsLando
left a comment
There was a problem hiding this comment.
Well done with this @seriousme ! You used a clever approach with the separated asyncPersistence and I also like a lot that lot of deps have been removed. KUDOS 💪🏼
|
I think I have applied all the suggested changes. |
|
ps. I looked at the coverage report as well and most of the missing lines and branches are in persistence.js and like: if (!this.ready) {
this.once('ready', this.storeRetained.bind(this, packet, cb))
return
}asyncPersistence.js has nearly 100% coverage. |
Thx, its been quite a journey updating all dependencies , and some of their dependencies as well 😉 Just let me know what you guys want. I like solving the puzzles 😄 Kind regards, |
|
One more weird thought: now that |
I think that's a good idea! @mcollina thoughts?
Absolutely, NodeJS 18 will soon reach EOL (the end of this month) so I think it's a good time to do it |
|
Seems like GitHub had an issue with starting MongoDB, can someone restart the tests? I just tried again using github codespaces and testing works there. Kind regards, |
|
@seriousme Done, could you add a timeout minutes on tests please? I think 3 minutes are enough? |
|
@seriousme I have a feel there is a race condition where the docker container is not ready before test runs, I suggest to use https://github.com/vishnubob/wait-for-it |
|
Instead of adding yet another dependency I have added I also set a generic timeout of 3 minutes for all tests as requested. Kind regards, |
|
@seriousme I also sent you an invitation to join our org, would you accept it? |
|
For some reason tests are still failing :( |
What does joining the org mean? I.e. what do you expect from me if I join ? |
It seems like the test still starts with abs.js instead of _connected.js. |
I personally don't expect nothing, there are other people in the org that are not active in years, considering you are an active contributor I think it makes sense to have you in the org, being part of the org means you don't have to wait me to approve running actions on your PR and you can work directly on repos branches insted of your own fork :) Also it's kind of an acknowledgment for the help you are giving to our org 🙏🏼 |
|
@seriousme I meant aedes-cached-persistence, not this, you should bump cached persistence here I think? And remove the custom |
|
Right, best option would imo be to first do another release of aedes-persistence, I can then update aedes-cached-persistence to take advantage of the promisified.js in aedes-persistence. |
It's ok for me 👍🏼 |
|
Back on the wild goose chase again. I tried reverting to the commit that passed CI above (rebuilding node_modules) and even that now hangs on the second test in own.js. Any hints would be much apreciated ;-) Kind regards, |
| function waitForEventOnce (emitter, eventName, errorEvent) { | ||
| return new Promise((resolve, reject) => { | ||
| emitter.once(eventName, resolve) | ||
| if (errorEvent) { | ||
| emitter.once(errorEvent, reject) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
you can replace this with once imported from node:events/promises
There was a problem hiding this comment.
Node 22.14.0 says: Error [ERR_UNKNOWN_BUILTIN_MODULE]: No such built-in module: node:events/promises
| const addSubs1 = this.asyncPersistence.addSubscriptions(client, subs) | ||
| // promisify | ||
| const addSubs2 = new Promise((resolve, reject) => { | ||
| this._addedSubscriptions(client, subs, (err) => { | ||
| if (err) { | ||
| reject(err) | ||
| } else { | ||
| resolve() | ||
| } |
There was a problem hiding this comment.
this is wrong as you should ensure the first completes before the second one
There was a problem hiding this comment.
I fixed this one and removeSubscriptions but test/own.js still fails and test/abs.js still succeeds.
There was a problem hiding this comment.
I fixed it in moscajs/aedes-cached-persistence#61, once mqemitter-mongodb releases a new version I can use callBackPersistence.js from the updated aedes-cached-persistence and things will run after each other.
| const remSubs1 = this.asyncPersistence.removeSubscriptions(client, subs) | ||
| // promisify | ||
| const mappedSubs = subs.map(sub => { return { topic: sub } }) | ||
| const remSubs2 = new Promise((resolve, reject) => { | ||
| this._removedSubscriptions(client, mappedSubs, (err) => { | ||
| if (err) { | ||
| reject(err) | ||
| } else { | ||
| resolve() | ||
| } | ||
| }) |
| try { | ||
| await ctx._cl.retained.bulkWrite(operations) | ||
| } catch (err) { | ||
| // ingnore error | ||
| } |
There was a problem hiding this comment.
| try { | |
| await ctx._cl.retained.bulkWrite(operations) | |
| } catch (err) { | |
| // ingnore error | |
| } | |
| await ctx._cl.retained.bulkWrite(operations).catch(() => {}) |
|
Small update: when using mongosh and running own.js I see the record stored in the collection 'pubsub' , but somehow the emitter is not picking it up. I wiretapped mqemitter-mongodb.js to trace any records coming in via: const cursor = await that._collection.find({ _id: { $gt: that._lastObj._id } }, {
tailable: true,
timeout: false,
awaitData: true
})When running abs.js I see packets being picked up by the cursor, but when using own.js I don't see them coming in. I'm going to check these. I'm still in the dark how 3442cef made it through CI, because when I check it out in a new codespace now and run the test then abs.js succeeds and own.js fails, so it must be something related to Mongodb or timing. To be continued. |
|
@seriousme Could it be that maybe the database drop causes mqemitter to loose the tracking of the collection someway? |
|
@robertsLando I traced it back to a change in the Mongo:8 container. I checked out mqemitter-mongodb version that passed CI and Matteo released some weeks ago and that now fails too. MongoDB@6 has some challenges around tailable cursors when the collection is empty. Kind regards, |
Nice finding! I remember a while ago I had some headaches to fix an issue on mqemitter mongo to ensure packet were processed in order, that's why I introduced ordered bulks there and also here then as MongoDB was emitting packets in the wrong order. Hopefully they will be able to help you track down this issue, keep me updated 🙏🏼 |
I was wrong, It turned out to be a bug in my upgrade to mongodb@6 of mqemitter-mongodb (see issue) How that ever got past CI (at least 3 times) is a mystery to me, but it is solved now. As soon as a new version of mqemitter-mongodb is released I can update this branch and we should finally be done here (fingers crossed). I already did a dryrun with a local copy of the updated mqemitter-mongodb and it passed all the tests. Kind regards, |
|
@robertsLando : CI is green again after upgrading mqemitter-mongodb to 9.0.1. Once Kind regards, |
|
@seriousme I tried to release the package but tests are failing locally. First test that fails is: Then it hangs indeterminately on: Any thoughts? Node version is 20.18.1, also tried with 20.19.1 and 22.15.0 without success |
|
Sorry to read that the test failed 😞 I just created a clean Codespace on master and tests pass, so troubleshooting will be a bit dificult. Does it consitently fail on your machine? The source of the error is: https://github.com/mcollina/mqemitter-mongodb/blob/97b7703cf691f0ac3aff104e1b9569d3eac92989/mqemitter-mongodb.js#L177 The stream is started in the setup phase of mqemitter-mongodb, 'do not error if unkown messageId in outoingClearMessageId' is the last test of abstract.js and if cleanup of one of the previous tests failed then the test suite will need to wait for timeout. Setup and teardown is identical for all tests in abstract.js so I can't explain why it only fails at the 10th test. If it consistently fails at the 10th test then from the top of my head I can think of the following options:
docker logs mongodb might give you a hint as to why the connection is dropped, but they are not very readable :-( Hope this helps, |
|
@seriousme let me try to run the release from a codespace first :) |
|
Ok so the tests are working on codespace, no clue why, I tried everything locally but I'm not able to make them work, who knows 🤷🏼♂️ BTW the release commit CI failed, I tried to re-run jobs now. Seems tests are a bit flaky now, no clue why but I don't want you to invest too much time on this as it has been already a pain |
|
I think adding a (fraction of a) second after cleaning the DB might help as this would allow Mongodb to catchup we could add an: await sleep(500) just after: aedes-persistence-mongodb/test/own.js Lines 31 to 35 in 67f01f3 and after aedes-persistence-mongodb/test/abs.js Lines 11 to 13 in 67f01f3 This would require adding It's a bit hard to test as I haven't had failing tests yet, but if it consistently fails on your machine then this would be my first guess. Kind regards, |
|
It also failed on master branch, check last ci runs. But maybe the reason is different: https://github.com/moscajs/aedes-persistence-mongodb/actions/runs/14990090351/job/42112474933#step:7:103 Seems that the packets are actually equal but not strictly as props are in a different order? But here the error still looks like the same I get on my machine: https://github.com/moscajs/aedes-persistence-mongodb/actions/runs/14990090351/job/42111317625#step:7:111 Might be worth a try adding a sleep I think |
|
Now this is interesting: the order of the props is not the issue I think, but the actual has an "added: 2025-05-13T07:13:21.450Z" property which the expected does not have. My codespace uses Mongodb 8.0.9 , checked using |
|
The second error is indeed the same error: 'Lost connection to MongoDB' in test 10. I will add the sleeps and see what that does. |
Oh that key is added when a TTL is set for that collection, maybe you should also drop indexes when cleaning the db? Dunno if when you delete a collection also indexes are dropped (I think so but who knows 🤷🏼♂️ ) |
3 participants
This PR closes: #82
It updates package.json
It updates CI to use:
It splits testing in separate files to ease test debugging
It creates a clean interface between the callback world of aedes-persistence and the async world of mongodb@6
It migrates testing to async because of mongodb@6 and caugth a few bugs in the tests that previously got lost in the callbacks.
It updates the README to fix the link to the badge.
Enjoy!
Kind regards,
Hans