Skip to content
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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions js/sdk/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion js/sdk/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -91,5 +91,5 @@
"build:docs": "rm -rf ../../web/developer.actyx.com/static/@actyx/pond/ && typedoc --mode library --options typedoc.json src/index.ts"
},
"types": "./lib/index.d.ts",
"version": "0.5.9"
"version": "0.5.10"
}
102 changes: 92 additions & 10 deletions js/sdk/src/actyx.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 creations of actyx instance with these parameters are detected! manifest: ${JSON.stringify(
Kelerchian marked this conversation as resolved.
Show resolved Hide resolved
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 = () => {
const result = oldDispose()
actyxInstanceRegister.subOne(manifest, opts)
Copy link
Member

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 the subOne before the oldDispose.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed fc39630

return result
}
actyxInstanceRegister.addOne(manifest, opts)

return actyx
},

/**
Expand Down Expand Up @@ -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]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dislike the Param name, Params/Parameters makes much more sense given it's a list.

I'd also add a brief docstring

Copy link
Member

Choose a reason for hiding this comment

The 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 }
})()