-
-
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 all 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,14 +1,13 @@ | ||
| import fastfall from 'fastfall' | ||
| 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' | ||
|
|
||
| const subscribeTopicActions = fastfall([ | ||
| const subscribeTopicActions = runFall([ | ||
| authorize, | ||
| storeSubscriptions, | ||
| addSubs | ||
| ]) | ||
| const restoreTopicActions = fastfall([ | ||
| const restoreTopicActions = runFall([ | ||
| authorize, | ||
| addSubs | ||
| ]) | ||
|
|
@@ -78,20 +77,30 @@ function _dedupe (subs) { | |
| return ret | ||
| } | ||
|
|
||
| function handleSubscribe (client, packet, restore, done) { | ||
| async function handleSubscribe (client, packet, restore, done) { | ||
| packet.subscriptions = packet.subscriptions.length === 1 ? packet.subscriptions : _dedupe(packet.subscriptions) | ||
| client.broker._parallel( | ||
| new SubscribeState(client, packet, restore, done), // what will be this in the functions | ||
| doSubscribe, // function to call | ||
| packet.subscriptions, // first argument of the function | ||
| restore ? done : completeSubscribe // the function to be called when the parallel ends | ||
| ) | ||
| const state = new SubscribeState(client, packet, restore, done) | ||
| try { | ||
| 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 { | ||
| completeSubscribe.call(state) | ||
| } | ||
| } catch (err) { | ||
| done(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. 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) { | ||
|
|
@@ -159,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) | ||
|
|
@@ -185,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 | ||
|
|
||
|
|
||
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.