feat: asyncify #105
feat: asyncify #105
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces async/await functionality across the persistence layer for the MQTT broker, enabling both callback‐ and promise‐based interfaces while remaining backwards compatible. Key changes include the addition of an asyncPersistence.js backend, updates to the callback and promisified layers, and corresponding adjustments to tests and TypeScript definitions.
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| types/asyncPersistence.test-d.ts | Provides TypeScript tests for the new async persistence functionality. |
| test/*.js | Updated and new test files to validate both async and callback persistence interfaces. |
| promisified.js | Modified promise-wrapping logic to adjust return formats for async methods. |
| persistence.js | Refactored to use the callback layer that now leverages the new async persistence backend. |
| callBackPersistence.js | Updated to support async setup and method chaining while retaining callback support. |
| broadcastPersistence.js | Added a new module for handling subscription broadcast functionality asynchronously. |
| asyncPersistence.js | Introduces a fully async persistence backend with async/await and error handling enhancements. |
| abstract.js | Updated test abstractions to call helper functions that align with the new async interfaces. |
| package.json | Adjustments to test and coverage scripts and dependency updates to support the async functionality. |
asyncPersistence.js
Outdated
| return | ||
| } | ||
| } | ||
| throw new Error('no such packet here') |
There was a problem hiding this comment.
Consider standardizing the error message here to match the 'no such packet' text used in related methods for consistency across packet lookup failures.
| throw new Error('no such packet here') | |
| throw new Error('no such packet') |
Copilot uses AI. Check for mistakes.
abstract.js
Outdated
| // destroyed while it's still being set up. | ||
| // https://github.com/mcollina/aedes-persistence-redis/issues/41 | ||
| if (waitForReady && !instance.ready) { | ||
| await once(instance, 'ready') |
There was a problem hiding this comment.
I'm thinkiong about using promise race here with a timeout to be sure for whatever reason this hangs indeterminately?
There was a problem hiding this comment.
Added, although it will only be used during testing and we have node --test --test-timeout=180000 test/test.js on the other 3 persistence implementations now. Still it's a nice addition and we might be able to reuse the trick later in time.
asyncPersistence.js
Outdated
| #retained | ||
| #subscriptions | ||
| #outgoing | ||
| #incoming | ||
| #wills | ||
| #clientsCount | ||
| #destroyed | ||
| #broadcastSubscriptions | ||
| _trie | ||
| broker |
There was a problem hiding this comment.
FYI you can use /** @type { ... } */ to declare types in a non TS env, maybe it could be useful here :)
There was a problem hiding this comment.
Do we want to add full JSdoc, or even move to Typescript ? (e.g. see Opifex 😉 )
Main reason to list _trie and broker here was to not forget about them as they used to be injected without setters and getters. I have made them private now and added the getter for #broker that is currently required for testing. It is probably possible to refactor the tests so that the broker does not need to be exposed, but for now this works. I checked Aedes and it does not seem to "get" the broker from persistence.
There was a problem hiding this comment.
Do we want to add full JSdoc, or even move to Typescript ?
Moving to TS would be awesome but would require lot of work. I let you do the choice here :) Maybe with the new AI agents it could be much faster and easier now, still have to try
There was a problem hiding this comment.
I see it, are you using Aedes there? I'm also the maintainer of MQTTjs, wondering why you built your own MQTT server/client? Mine is just a curiosity 😄
There was a problem hiding this comment.
Opifex does not have any external dependencies. I wanted to learn Typescript and was wondering how hard it would be to build your own MQTT server, so like you: curiosity 😄
When I started Deno was the only easy route to Typescript but had slightly different sockets from Nodejs. These days Deno and Bun support Nodejs sockets as well.
Opifex is written on top of webstreams, this turned out to be a slight challenge on Nodejs as ReadableWebstream on Nodejs do not support bytestream (yet).
Main difference in packet handling compared to mqtt-packet (last time I looked at it) is that Opifex will read the packet size, the read a buffer of size packetsize and then parses the packet in one go.
And of course its using async/await, (async) iterables and generators where possible.
Feel free to ask more questions 😉
There was a problem hiding this comment.
Was just wondering if you could be interested in improving MQTTjs as well, starting from mqtt-packet and then the client as well, my idea is to drop as much deps as possible from it and make it more browser friendly :) First thing I did when I started as contributor has been to completely rewrite it to TS and that helped me to have a starting point from where making changes to the code is much less error prone and also helped me understanding the codebase better (and fix tons of bugs as well). Let me know if you want to continue this topic, of course like with Aedes there would be 0 expectations, just do whatever you want when you want (the same I do, no way pays me to support that)
There was a problem hiding this comment.
I created a discussion item on MQTTjs, lets discuss further there.
mqttjs/MQTT.js#2000
|
I have implemented the requested updates as discussed above and ran the tests again on mongo,redis and level which all passed. |
|
@seriousme Can I merge and release? |
This PR addresses #104 and provides:
It removes the need for
aedes-cached-persistence.The whole system has been tuned/tested with:
These new branches currently depend on:
"aedes-persistence": "github:seriousme/aedes-persistence#asyncify"as can be seen in their package.json's , this allowed me to build and test without a new release being available yet.
If this PR is merged it is easy to change this dependency to a new version of
aedes-persistenceKind regards,
Hans