-
Notifications
You must be signed in to change notification settings - Fork 7
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
add warning to multiple sdk instances with the same parameters #566
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -123,18 +123,43 @@ export const Actyx = { | |
const nodeId = await v2getNodeId(opts) | ||
log.actyx.debug('NodeId call returned:', nodeId) | ||
|
||
if (!nodeId) { | ||
// Try connecting to v1 if we failed to retrieve a v2 node id | ||
// (Note that if the port is completely unreachable, v2getNodeId will throw an exception and we don’t get here.) | ||
log.actyx.debug('NodeId was null, trying to reach V1 backend...') | ||
return createV1(opts) | ||
const previousInstanceCount = actyxInstanceRegister.count(manifest, opts) | ||
if (previousInstanceCount > 0) { | ||
console.warn( | ||
new Error( | ||
`Multiple connections to Actyx with these parameters are detected! manifest: ${JSON.stringify( | ||
manifest, | ||
)} opts: ${JSON.stringify(opts)}`, | ||
), | ||
) | ||
} | ||
|
||
log.actyx.debug( | ||
'Detected V2 is running, trying to authorize with manifest', | ||
JSON.stringify(manifest), | ||
) | ||
return createV2(manifest, opts, nodeId) | ||
// Create actyx instance | ||
const actyx = await (() => { | ||
if (!nodeId) { | ||
// Try connecting to v1 if we failed to retrieve a v2 node id | ||
// (Note that if the port is completely unreachable, v2getNodeId will throw an exception and we don’t get here.) | ||
log.actyx.debug('NodeId was null, trying to reach V1 backend...') | ||
return createV1(opts) | ||
} | ||
|
||
log.actyx.debug( | ||
'Detected V2 is running, trying to authorize with manifest', | ||
JSON.stringify(manifest), | ||
) | ||
return createV2(manifest, opts, nodeId) | ||
})() | ||
|
||
// Patch dispose | ||
const oldDispose = actyx.dispose | ||
actyx.dispose = () => { | ||
actyxInstanceRegister.subOne(manifest, opts) | ||
const result = oldDispose() | ||
return result | ||
} | ||
actyxInstanceRegister.addOne(manifest, opts) | ||
|
||
return actyx | ||
}, | ||
|
||
/** | ||
|
@@ -163,3 +188,60 @@ export const Actyx = { | |
} | ||
}, | ||
} | ||
|
||
/** | ||
* "Reference counter" for living Actyx | ||
*/ | ||
const actyxInstanceRegister = (() => { | ||
type AppId = AppManifest['appId'] | ||
type Version = AppManifest['version'] | ||
type Signature = AppManifest['signature'] | ||
type ActyxHost = ActyxOpts['actyxHost'] | ||
type ActyxPort = ActyxOpts['actyxPort'] | ||
type Param = [AppId, Version, Signature, ActyxHost, ActyxPort] | ||
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 dislike the I'd also add a brief docstring 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. these types aren’t visible to anyone, so a docstring won’t be displayed anywhere — did you mean a comment? |
||
|
||
const register: Map<Param, number> = new Map() | ||
|
||
const toParam = (manifest: AppManifest, opts: ActyxOpts): Param => [ | ||
manifest.appId, | ||
manifest.version, | ||
manifest.signature, | ||
opts.actyxHost, | ||
opts.actyxPort, | ||
] | ||
|
||
const paramEq = (paramA: Param, paramB: Param) => { | ||
for (let i = 0; i < Math.max(paramA.length, paramB.length); i++) { | ||
if (paramA[i] !== paramB[i]) { | ||
return false | ||
} | ||
} | ||
return true | ||
} | ||
|
||
const findEntry = (manifest: AppManifest, opts: ActyxOpts) => { | ||
const entries = Array.from(register.entries()) | ||
const param = toParam(manifest, opts) | ||
const matchingEntry = entries.find((x) => { | ||
return paramEq(x[0], param) | ||
}) | ||
if (!matchingEntry) { | ||
return [param, 0] as const | ||
} | ||
return matchingEntry | ||
} | ||
|
||
const addOne = (manifest: AppManifest, opts: ActyxOpts) => { | ||
const [param, refcount] = findEntry(manifest, opts) | ||
register.set(param, refcount + 1) | ||
} | ||
|
||
const count = (manifest: AppManifest, opts: ActyxOpts) => findEntry(manifest, opts)[1] | ||
|
||
const subOne = (manifest: AppManifest, opts: ActyxOpts) => { | ||
const [param, refcount] = findEntry(manifest, opts) | ||
register.set(param, refcount - 1) | ||
} | ||
|
||
return { addOne, count, subOne } | ||
})() |
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.
Does
oldDispose()
invoke some user callbacks? If so, such callback could try to create a new SDK instance, which in that case would log the “multiple connections” error. If correctness permits it, I’d do thesubOne
before theoldDispose
.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.
It only calls
MultiplexedWebsocket.dispose
under the hood.There should be no difference between both orders since this is a sync function.
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.
My worry was that there might be an event emitted to user code from that sync function — and if we add such a thing in the future then this problem may well surface.
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.
Ah, that's a valid concern
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.
fixed fc39630