chore: reduce production dependencies#1045
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1045 +/- ##
===========================================
+ Coverage 99.47% 100.00% +0.52%
===========================================
Files 15 15
Lines 1921 1975 +54
===========================================
+ Hits 1911 1975 +64
+ Misses 10 0 -10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Automated Benchmark testing suggests more than 10℅ perf decrease. Kind regards, |
|
It turns out the difference was actually positive instead of negative, tested on Codespaces using 4 cores. Overall Benchmark Results+x% is better, -x% is worse, current threshold to fail at -10%
Benchmark Results for main
Benchmark Results for reduce-production-dependencies
|
|
A fix for the benchmarking is in #1046 Kind regards, |
robertsLando
left a comment
robertsLando
left a comment
There was a problem hiding this comment.
Love this! ❤️
Let's see if @mcollina has some opinions on this otherwise this is good to merge 🙏🏼
There was a problem hiding this comment.
Pull Request Overview
This PR reduces production dependencies by replacing 8 external packages with native Node.js APIs and local implementations. The goal is to minimize external dependencies while maintaining equivalent functionality.
- Replaces external utilities like
fastfall,fastparallel,fastserieswith local implementations - Switches from
hyperidanduuidpackages to nativerandomUUIDfrom Node.js crypto module - Replaces
end-of-stream,retimer, andreusifywith native or local alternatives
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Removes 8 production dependencies |
| lib/utils.js | Adds local ObjectPool and retimer implementations |
| lib/handlers/unsubscribe.js | Replaces fastparallel with Promise.all pattern |
| lib/handlers/subscribe.js | Adds runFall implementation and replaces fastparallel with Promise.all |
| lib/handlers/connect.js | Switches from hyperid to native randomUUID |
| lib/client.js | Replaces end-of-stream with native finished from stream |
| docs/Client.md | Updates documentation to reflect hyperid → randomUUID change |
| aedes.js | Replaces multiple external dependencies with native/local implementations |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
mcollina
left a comment
mcollina
left a comment
There was a problem hiding this comment.
I generically don't see a problem with dependencies and number of things on disk, but some of the changes are incorrect or will worsen the performance.
Some of it is in the right direction because it will now be using things from the platform.
Kind regards, |
|
@robertsLando
|
|
@robertsLando The only file without 100% coverage is now client.js because of : Lines 377 to 379 in 9b76359 which we can't test and therefore should imo exclude from coverage testing. And legacy support in: Lines 169 to 172 in 9b76359
The associated drain request: Lines 362 to 364 in 9b76359 is never called according to coverage testing. Calling nodejs private API's like this.conn._writableState.getBuffer() should not be required, so Iḿ really wondering whether we should not leave this to conn.destroy(). Kind regards, |
| }) | ||
| }))) | ||
| if (restore) { | ||
| done() |
There was a problem hiding this comment.
Note that if this throws, it would call done twice
There was a problem hiding this comment.
I don't see why, can you please explain?
I tried the following:
const pOk = new Promise(resolve => {
console.log('pOK will resolve')
resolve()
})
const pFail = new Promise((resolve,reject) => {
console.log('pFail will reject')
reject('pFail rejected')
})
try {
await Promise.all([pOk,pFail])
console.log('all Promises resolved ok')
} catch (err){
console.log('At least one promise failed, first error:', err)
}This gives me:
pOK will resolve
pFail will reject
At least one promise failed, first error: pFail rejected
Btw: I reworked doSubscribe to make this part more easy to read.
There was a problem hiding this comment.
@seriousme — the concern is not the Promise.all([ok, fail]) resolution shape (you're right that the catch fires once). The issue is what happens after await Promise.all(...) resolves and completeSubscribe.call(state) runs.
write() in lib/write.js always uses setImmediate(done, error, client) — so done is scheduled but not yet invoked. If anything in completeSubscribe throws synchronously after that write() call (a listener installed on the 'subscribe' event throws, broker.publish(...) throws inside an mq listener, persistence.createRetainedStreamCombi throws, etc.), the error escapes the try block:
try {
await Promise.all(...)
completeSubscribe.call(state) // queues done via write(), then throws
} catch (err) {
done(err) // catch calls done
}
// next tick: setImmediate fires → done called AGAINMinimal repro:
const state = { finish: function(err){ console.log('DONE', err?.message ?? 'OK') } }
function write (cb) { setImmediate(cb) }
function completeSubscribe () {
write(state.finish)
throw new Error('listener threw')
}
async function doSub () { return Promise.resolve() }
async function handleSubscribe () {
try {
await Promise.all([doSub()])
completeSubscribe.call(state)
} catch (err) {
state.finish(err)
}
}
handleSubscribe()
// → DONE listener threw
// → DONE OK ← second invocationFix direction: move the post-write work outside the try block, or wrap done in an idempotent guard before passing it through state.finish.
| // since it is a rare race condition we ignore it in coverage testing | ||
| return | ||
| } | ||
| /* c8 ignore stop */ |
There was a problem hiding this comment.
Not that I know, the coverage testing shows that this part is never reached and given the comments it looked like a rare race condition to me.
There was a problem hiding this comment.
Marking this as a follow-up rather than a blocker, but worth a thought: the current /* c8 ignore */ block is justified with "rare race condition" — that's a behavioral claim that should age well, and currently the comment doesn't show the race. Two options:
- If
client.closed || broker.closedcan be reached here, drop a one-liner pointing at the racing call site (e.g. "set inaedes.close()whileauthorizeSubscribeis in flight; see test X"), and ideally write the regression test. - If it's structurally unreachable (because
authorizeSubscribeandclose()are serialized somewhere upstream), then the dead branch should be removed, not ignored.
Not blocking the PR, but "rare race we ignore in coverage" is the kind of comment that rots — better to commit either way.
|
FYI: with all the changes the github review comments are a bit hard to track. |
|
@robertsLando @mcollina Kind regards, |
|
@mcollina ping |
|
Just to be sure: you all are not waiting for me right? |
|
@seriousme nope, I'm waiting for @mcollina last feedback after your changes after his review :) |
|
any news? npm audit now warns about outdated version of uuid used by hyperid |
|
@seriousme I'm doing a review now, if that's ok I will merge it |
robertsLando
left a comment
robertsLando
left a comment
There was a problem hiding this comment.
Deep code review
Verdict: Needs work — two real correctness bugs in the new Promise.all-based error paths (both flagged previously by @mcollina, both reproducible locally).
Top 3 risks:
unsubscribe.js— caller hangs onPromise.allrejection:completeUnsubscribe(err)emits'error'and returns without invokingdone. → see inline thread reply.subscribe.js—donecan be called twice ifcompleteSubscribethrows afterwrite()has queuedsetImmediate(done, …). → see inline thread reply.handlers/index.js:64—client._keepaliveTimer.refresh(client._keepaliveInterval)— Node'sTimeout.refresh()is nullary; the interval arg is silently dropped. → see inline finding.
Strengths:
- Direction is sound: replacing micro-deps with platform APIs (
randomUUID,finished,Timeout.refresh) reduces supply-chain surface; benchmark shows ~+15–18% msg/s at QoS 0. batch()async generator gives bounded concurrency (default 16) and is genuinely more readable than the prior fastfall/fastparallel chain.runFall/batchget standalone tests intest/utils.js.randomUUIDis more secure thanhyperid(cryptographic vs. predictable); perf delta is irrelevant at once-per-connection cost.- ObjectPool removal validated by benchmark — @mcollina's hypothesis confirmed.
Mcollina's review comments — verification:
| Comment | Status |
|---|---|
aedes.js:141 "bazillion of promises" |
Addressed — batch() (bounded concurrency, default 16). ✅ resolving |
lib/utils.js:70 "use timer.refresh" / "remove utility" |
Addressed — retimer utility removed entirely. ✅ resolving |
lib/utils.js:59 "ObjectPool worse than reallocating" |
Addressed — removed. ✅ already resolved |
aedes.js:57 "verify ObjectPool needed at all" |
Addressed — removed; benchmark confirmed. ✅ resolving |
aedes.js:138 "process.nextTick(done)" |
Addressed — async/await removed the callback path. ✅ resolving |
aedes.js:261 "callback not invoked if one fails" |
Addressed — Promise.all(...).finally(...). ✅ resolving |
subscribe.js:22 "Please test this" |
Addressed — runFall standalone tests in test/utils.js. ✅ resolving |
lib/utils.js:84 "benchmark vs hwp" |
Addressed — benchmark posted in-thread. ✅ resolving |
lib/utils.js:75 "error handling here" |
Addressed — batch() error-path test added. ✅ resolving |
subscribe.js:86 "throws → calls done twice" |
❌ NOT FIXED — reproducible; see thread reply. |
subscribe.js:92 / unsubscribe.js:50 "same problem with promises" |
❌ NOT FIXED — unsubscribe path never calls done on rejection; see thread replies. |
subscribe.js:179 "was this tested" |
❌ NOT TESTED — explicitly c8 ignored; comment to follow up. |
Copilot +1s (connect.js:84, client.js:10) |
Addressed. |
This PR is net positive and should land — the supply-chain reduction is the right move and the perf result is a nice bonus — but the two error-path bugs need fixing first. Both are mcollina's prior concerns and both are reproducible in ~20 lines.
|
@robertsLando |
|
@seriousme I always do a review myself and check both ai review, sometimes I pick something it miss sometimes the opposite, it's just a plus :) |
robertsLando
left a comment
robertsLando
left a comment
There was a problem hiding this comment.
@seriousme Please check the 3 missing open review threads
|
@robertsLando : the problem I have with Copilot is that its hard to tell which comments are yours and which are the LLM's |
|
@seriousme yep understand that. BTW did you saw the 3 open threads I commented on? |
|
Yes, did you see my replies? |
|
Thanks for making efforts. |
|
@seriousme nope I don't see them |
|
@robertsLando : strange must be a github glitch as I see them when opening a new browser window and looking at the PR. Anyway the subscribe and unsubscribe ones I fixed, you then added "cc @seriousme " after my "fixed" to which I replied "???" as I did not understand what you ment with the "cc" The c8 exclusion I left in as there is no test that covers this line and think I can only refactor it out if I change much more and the aim was to change as little as possible to keep the impact of the changes as small as possible. Should I mark them as resolved myself? Or do you want to check and mark them as resolved? |
|
Ok, while checking if I could generate a test to solve the "c8 ignore" (which worked) I noticed that the fixes you requested add new lines not covered by testing , especially the double done scenarios. And somehow also a line in client.js. I will see if I can get these covered as well. If not I will add "c8 ignore again" |
|
Ok, I added extra tests to get coverage back to 100% again. To do that I also had to modernize the socket destruction code which relied on the internals of nodejs As far as I know everything is in now. If I still missed anything please let me know. Kind regards, |
| const state = this.conn._writableState | ||
| const list = typeof state.getBuffer === 'function' ? state.getBuffer() : state.buffer | ||
| list.forEach(drainRequest) |
There was a problem hiding this comment.
I know this was an old hacky hack to fix an annoying but, wondering if it's still legit or not, no clue if we ever have a test that covered this
robertsLando
left a comment
robertsLando
left a comment
There was a problem hiding this comment.
Two follow-ups on the error-path fixes from e366386.
| } | ||
|
|
||
| function completeSubscribe () { | ||
| const done = once(this.finish) |
There was a problem hiding this comment.
The wrap only covers calls that go through this local done. The catch in handleSubscribe (L91) still has the outer done in scope and calls it directly, so the original double-call path is still reachable: write(client, …, done) queues setImmediate(done, …), then anything throwing synchronously below — a listener on 'subscribe', broker.publish(…) reaching a throwing emitter, the stream.pipe setup — escapes back to the catch, which fires done(err); then the queued tick fires done(null, client). The flag in once() is on the wrapped reference; the catch path never touches that closure.
If you wrap one level up — done = once(done) at the top of handleSubscribe and drop the local wrap here — both paths share the same gate and the bug is closed.
Repro is ~15 lines if it helps:
function once (fn) { let c = false; return e => { if (c) return; c = true; fn(e) } }
const state = { finish: e => console.log('DONE', e?.message ?? 'OK') }
function completeSub () {
const done = once(state.finish)
setImmediate(done) // mimics write(...,done)
throw new Error('listener threw') // anything sync after write
}
async function handleSub (done) {
try { await Promise.resolve(); completeSub() }
catch (err) { done(err) } // bypasses once()
}
handleSub(state.finish)
// → DONE listener threw
// → DONE OK| await Promise.all(packet.unsubscriptions.map(sub => doUnsubscribe(state, sub))) | ||
| completeUnsubscribe.call(state) | ||
| } catch (err) { | ||
| completeUnsubscribe.call(state, err) |
There was a problem hiding this comment.
The new return done(err) in completeUnsubscribe closes the "caller hangs" path on Promise.all rejection — good.
But the symmetric case now opens up the same shape as the subscribe thread: on the success path, write(client, UnSubAck, done) (L89) queues setImmediate(done), then client.broker.emit('unsubscribe', …) (L96) can throw if a listener throws — escapes completeUnsubscribe, lands here in the catch → completeUnsubscribe.call(state, err) → done(err) at L83. The queued tick then fires done(null, client) a second time.
Same fix shape: wrap done once at the top of actualUnsubscribe (done = once(done)) so both branches share one gate. The once helper added in subscribe.js could move to utils.js and be reused here.
@robertsLando
As promised, this PR reduces the number of production dependencies to a bare minimum.
This PR replaces:
The local variants listed above are all quite small, mostly because they only need to support one variant instead of the many variants that some external modules provide. Also over the years some patterns have become more easy to implement with modern Javascript.
This PR tries to keep the code changes to a minimum.
From here on, further optimizations might be possible, but I didn't want to make this PR too complex.
Kind regards,
Hans