-
-
Notifications
You must be signed in to change notification settings - Fork 239
chore: reduce production dependencies #1045
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
base: main
Are you sure you want to change the base?
Changes from 8 commits
c0f3047
e880e5f
9e5d2c7
f4aebec
3d7fd28
1b3bb3d
cb8ca24
ac207b0
a86f22d
6fed21d
c2cc706
e8d6a4b
bfeb6ca
3a35a63
31a9bc1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,26 +1,7 @@ | ||
| import Packet from 'aedes-packet' | ||
| import { through, validateTopic, $SYS_PREFIX } from '../utils.js' | ||
| import { through, validateTopic, $SYS_PREFIX, runFall } from '../utils.js' | ||
| import write from '../write.js' | ||
|
|
||
| function runFall (fns) { | ||
| // run functions in fastfall style, only need the single argument function | ||
| return function (arg, cb) { | ||
| let i = 0 | ||
| const ctx = this | ||
| function next (err, nextarg) { | ||
| if (err || i === fns.length) { | ||
| if (typeof cb === 'function') { | ||
| cb.call(ctx, err, nextarg) | ||
| } | ||
| return | ||
| } | ||
| const fn = fns[i++] | ||
| fn.call(ctx, nextarg, next) | ||
| } | ||
| next(null, arg) | ||
| } | ||
| } | ||
|
|
||
| const subscribeTopicActions = runFall([ | ||
| authorize, | ||
| storeSubscriptions, | ||
|
|
@@ -100,12 +81,7 @@ async function handleSubscribe (client, packet, restore, done) { | |
| packet.subscriptions = packet.subscriptions.length === 1 ? packet.subscriptions : _dedupe(packet.subscriptions) | ||
| const state = new SubscribeState(client, packet, restore, done) | ||
| try { | ||
| await Promise.all(packet.subscriptions.map(sub => new Promise((resolve, reject) => { | ||
| doSubscribe.call(state, sub, (err, result) => { | ||
| if (err) reject(err) | ||
| else resolve(result) | ||
| }) | ||
| }))) | ||
| await Promise.all(packet.subscriptions.map(sub => doSubscribe(state, sub))) | ||
| if (restore) { | ||
| done() | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that if this throws, it would call done twice
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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: Btw: I reworked doSubscribe to make this part more easy to read. |
||
| } else { | ||
|
|
@@ -116,10 +92,15 @@ async function handleSubscribe (client, packet, restore, done) { | |
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This has the same problem with promises identified elsewhere
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This has the same problem with promises identified elsewhere |
||
| } | ||
|
|
||
| function doSubscribe (sub, done) { | ||
| const s = new SubState(this.client, this.packet, sub.qos, sub.rh, sub.rap, sub.nl) | ||
| this.subState.push(s) | ||
| this.actions.call(s, sub, done) | ||
| async function doSubscribe (state, sub) { | ||
| return new Promise((resolve, reject) => { | ||
| const s = new SubState(state.client, state.packet, sub.qos, sub.rh, sub.rap, sub.nl) | ||
| state.subState.push(s) | ||
| state.actions.call(s, sub, (err, result) => { | ||
| if (err) reject(err) | ||
| else resolve(result) | ||
| }) | ||
| }) | ||
| } | ||
|
|
||
| function authorize (sub, done) { | ||
|
|
@@ -187,12 +168,15 @@ function addSubs (sub, done) { | |
| func = blockDollarSignTopics(func) | ||
| } | ||
|
|
||
| /* c8 ignore start */ | ||
| if (client.closed || client.broker.closed) { | ||
| // a hack, sometimes client.close() or broker.close() happened | ||
| // before authenticate() comes back | ||
| // we don't continue subscription here | ||
| // since it is a rare race condition we ignore it in coverage testing | ||
| return | ||
| } | ||
| /* c8 ignore stop */ | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. was this tested before?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| if (!client.subscriptions[topic]) { | ||
| client.subscriptions[topic] = new Subscription(qos, func, rh, rap, nl) | ||
|
|
@@ -213,13 +197,8 @@ function isStartsWithWildcard (topic) { | |
| return code === 43 || code === 35 | ||
| } | ||
|
|
||
| function completeSubscribe (err) { | ||
| function completeSubscribe () { | ||
| const done = this.finish | ||
|
|
||
| if (err) { | ||
| return done(err) | ||
| } | ||
|
|
||
| const packet = this.packet | ||
| const client = this.client | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,33 +43,33 @@ function handleUnsubscribe (client, packet, done) { | |
| async function actualUnsubscribe (client, packet, done) { | ||
| const state = new UnsubscribeState(client, packet, done) | ||
| try { | ||
| await Promise.all(packet.unsubscriptions.map(sub => new Promise((resolve, reject) => { | ||
| doUnsubscribe.call(state, sub, (err, result) => { | ||
| if (err) reject(err) | ||
| else resolve(result) | ||
| }) | ||
| }))) | ||
| await Promise.all(packet.unsubscriptions.map(sub => doUnsubscribe(state, sub))) | ||
| completeUnsubscribe.call(state) | ||
| } catch (err) { | ||
| completeUnsubscribe.call(state, err) | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same problem
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same discussion as doSubscribe(), I also reworked doUnsubscribe to make the code more readable |
||
| } | ||
|
|
||
| function doUnsubscribe (sub, done) { | ||
| const client = this.client | ||
| const broker = client.broker | ||
| const s = client.subscriptions[sub] | ||
|
|
||
| if (s) { | ||
| const func = s.func | ||
| delete client.subscriptions[sub] | ||
| broker.unsubscribe( | ||
| sub, | ||
| func, | ||
| done) | ||
| } else { | ||
| done() | ||
| } | ||
| async function doUnsubscribe (state, sub) { | ||
| return new Promise((resolve, reject) => { | ||
| const client = state.client | ||
| const broker = client.broker | ||
| const s = client.subscriptions[sub] | ||
|
|
||
| if (s) { | ||
| const func = s.func | ||
| delete client.subscriptions[sub] | ||
| broker.unsubscribe( | ||
| sub, | ||
| func, | ||
| (err) => { | ||
| if (err) reject(err) | ||
| else resolve() | ||
| }) | ||
| } else { | ||
| resolve() | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| function completeUnsubscribe (err) { | ||
|
|
||
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.
This is by far not equivalent. It also creates a bazillion of promises and it will significantly impact performance
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.
You are right, the current solution is rather inefficient.
I'm working on a more elegant solution for this one.