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

Support retriable Chelonia actions #1768

Merged
merged 10 commits into from
Dec 7, 2023
9 changes: 8 additions & 1 deletion frontend/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import { LOGIN, LOGOUT, SWITCH_GROUP } from './utils/events.js'
import './controller/namespace.js'
import './controller/actions/index.js'
import './controller/backend.js'
import '~/shared/domains/chelonia/persistent-actions.js'
import manifests from './model/contracts/manifests.json'
import router from './controller/router.js'
import { PUBSUB_INSTANCE } from './controller/instance-keys.js'
Expand All @@ -43,6 +44,7 @@ const { Vue, L } = Common

console.info('GI_VERSION:', process.env.GI_VERSION)
console.info('CONTRACTS_VERSION:', process.env.CONTRACTS_VERSION)
console.info('LIGHTWEIGHT_CLIENT:', process.env.LIGHTWEIGHT_CLIENT)
console.info('NODE_ENV:', process.env.NODE_ENV)

Vue.config.errorHandler = function (err, vm, info) {
Expand Down Expand Up @@ -240,18 +242,23 @@ async function startApp () {
}
sbp('okTurtles.events/off', CONTRACT_IS_SYNCING, initialSyncFn)
sbp('okTurtles.events/on', CONTRACT_IS_SYNCING, syncFn.bind(this))
sbp('okTurtles.events/on', LOGIN, () => {
sbp('okTurtles.events/on', LOGIN, async () => {
this.ephemeral.finishedLogin = 'yes'

if (this.$store.state.currentGroupId) {
this.initOrResetPeriodicNotifications()
this.checkAndEmitOneTimeNotifications()
}
const databaseKey = `chelonia/persistentActions/${sbp('state/vuex/getters').ourIdentityContractId}`
sbp('chelonia.persistentActions/configure', { databaseKey })
await sbp('chelonia.persistentActions/load')
})
sbp('okTurtles.events/on', LOGOUT, () => {
this.ephemeral.finishedLogin = 'no'
router.currentRoute.path !== '/' && router.push({ path: '/' }).catch(console.error)
// Stop timers related to periodic notifications or persistent actions.
sbp('gi.periodicNotifications/clearStatesAndStopTimers')
sbp('chelonia.persistentActions/unload')
})
sbp('okTurtles.events/on', SWITCH_GROUP, () => {
this.initOrResetPeriodicNotifications()
Expand Down
2 changes: 1 addition & 1 deletion shared/domains/chelonia/db.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ const dbPrimitiveSelectors = process.env.LIGHTWEIGHT_CLIENT === 'true'
// eslint-disable-next-line require-await
'chelonia/db/set': async function (key: string, value: Buffer | string): Promise<Error | void> {
checkKey(key)
sbp('okTurtles.data/set', key, value)
return sbp('okTurtles.data/set', key, value)
},
// eslint-disable-next-line require-await
'chelonia/db/delete': async function (key: string): Promise<boolean> {
Expand Down
3 changes: 3 additions & 0 deletions shared/domains/chelonia/events.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,6 @@ export const CONTRACTS_MODIFIED = 'contracts-modified'
export const EVENT_HANDLED = 'event-handled'
export const CONTRACT_REGISTERED = 'contract-registered'
export const CONTRACT_UNREGISTERED = 'contract-unregistered'
export const PERSISTENT_ACTION_FAILURE = 'persistent-action-failure'
export const PERSISTENT_ACTION_SUCCESS = 'persistent-action-success'
export const PERSISTENT_ACTION_TOTAL_FAILURE = 'persistent-action-total_failure'
238 changes: 238 additions & 0 deletions shared/domains/chelonia/persistent-actions.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,238 @@
'use strict'

import sbp from '@sbp/sbp'
import '@sbp/okturtles.events'
import { PERSISTENT_ACTION_FAILURE, PERSISTENT_ACTION_SUCCESS, PERSISTENT_ACTION_TOTAL_FAILURE } from './events.js'

type SbpInvocation = any[]
type UUIDV4 = string

type PersistentActionOptions = {
errorInvocation?: SbpInvocation,
// Maximum number of tries, default: Infinity.
maxAttempts: number,
// How many seconds to wait between retries.
retrySeconds: number,
skipCondition?: SbpInvocation,
totalFailureInvocation?: SbpInvocation
}

type PersistentActionStatus = {|
attempting: boolean,
failedAttemptsSoFar: number,
lastError: string,
nextRetry: string,
resolved: boolean
|}

const coerceToError = (arg: any): Error => {
if (arg && arg instanceof Error) return arg
console.warn(tag, 'Please use Error objects when throwing or rejecting')
return new Error((typeof arg === 'string' ? arg : JSON.stringify(arg)) ?? 'undefined')
}

const defaultOptions = {
maxAttempts: Number.POSITIVE_INFINITY,
retrySeconds: 30
}
const tag = '[chelonia.persistentActions]'

class PersistentAction {
id: UUIDV4
invocation: SbpInvocation
options: PersistentActionOptions
status: PersistentActionStatus
timer: TimeoutID | void

constructor (invocation: SbpInvocation, options: PersistentActionOptions = {}) {
// $FlowFixMe: Cannot resolve name `crypto`.
this.id = crypto.randomUUID()
this.invocation = invocation
this.options = { ...defaultOptions, ...options }
this.status = {
attempting: false,
failedAttemptsSoFar: 0,
lastError: '',
nextRetry: '',
resolved: false
}
}

async attempt (): Promise<void> {
// Bail out if the action is already attempting or resolved.
// TODO: should we also check whether the skipCondition call is pending?
if (this.status.attempting || this.status.resolved) return
if (await this.trySBP(this.options.skipCondition)) this.cancel()
// We need to check this again because cancel() could have been called while awaiting the trySBP call.
if (this.status.resolved) return
try {
this.status.attempting = true
const result = await sbp(...this.invocation)
this.status.attempting = false
this.handleSuccess(result)
} catch (error) {
this.status.attempting = false
await this.handleError(coerceToError(error))
}
}

cancel (): void {
this.timer && clearTimeout(this.timer)
this.status.nextRetry = ''
this.status.resolved = true
}

async handleError (error: Error): Promise<void> {
const { id, options, status } = this
// Update relevant status fields before calling any optional code.
status.failedAttemptsSoFar++
status.lastError = error.message
const anyAttemptLeft = options.maxAttempts > status.failedAttemptsSoFar
if (!anyAttemptLeft) status.resolved = true
status.nextRetry = anyAttemptLeft && !status.resolved
Copy link
Member

Choose a reason for hiding this comment

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

!status.resolved could be ignored, I think.

status.nextRetry = anyAttemptLeft ? new Date(Date.now() + options.retrySeconds * 1e3).toISOString() : ''

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

status.resolved can be set independently by .cancel(), therefore checking for anyAttemptLeft was not enough

? new Date(Date.now() + options.retrySeconds * 1e3).toISOString()
Copy link
Member

Choose a reason for hiding this comment

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

Using Date is a bad idea for anything besides maybe logging. The reason is that the date could change arbitrarily. The better alternative is to use a monotonic timer for this, such as performance.now(). I'm not sure exactly what this nextRetry field is used for, exactly, though.

The second issue is the, like for the this.timer assignment below, that 1e3 seems like a pretty low value for a retry

Copy link
Collaborator Author

@snowteamer snowteamer Nov 7, 2023

Choose a reason for hiding this comment

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

The nextRetry field is actually used for logging as per the issue requirement for 'chelonia.persistentActions/status'

1e3 seems like a pretty low value for a retry

options.retrySeconds is a number of seconds, but we need milliseconds here, hence the 1e3 multiplier

: ''
// Perform any optional SBP invocation.
// The event has to be fired first for the action to be immediately removed from the list.
sbp('okTurtles.events/emit', PERSISTENT_ACTION_FAILURE, { error, id })
await this.trySBP(options.errorInvocation)
if (!anyAttemptLeft) {
sbp('okTurtles.events/emit', PERSISTENT_ACTION_TOTAL_FAILURE, { error, id })
await this.trySBP(options.totalFailureInvocation)
}
// Schedule a retry if appropriate.
if (status.nextRetry) {
// Note: there should be no older active timeout to clear.
this.timer = setTimeout(() => this.attempt(), this.options.retrySeconds * 1e3)
Copy link
Member

Choose a reason for hiding this comment

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

1e3 seems like a pretty low value

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ditto

}
}

handleSuccess (result: any): void {
const { id, status } = this
status.lastError = ''
status.nextRetry = ''
status.resolved = true
sbp('okTurtles.events/emit', PERSISTENT_ACTION_SUCCESS, { id, result })
}

async trySBP (invocation: SbpInvocation | void): Promise<any> {
try {
return invocation ? await sbp(...invocation) : undefined
} catch (error) {
console.error(tag, coerceToError(error).message)
}
}
}

// SBP API

sbp('sbp/selectors/register', {
'chelonia.persistentActions/_init' (): void {
this.actionsByID = Object.create(null)
this.checkDatabaseKey = () => {
if (!this.databaseKey) throw new TypeError(`${tag} No database key configured`)
}
sbp('okTurtles.events/on', PERSISTENT_ACTION_SUCCESS, ({ id }) => {
sbp('chelonia.persistentActions/cancel', id)
})
sbp('okTurtles.events/on', PERSISTENT_ACTION_TOTAL_FAILURE, ({ id }) => {
sbp('chelonia.persistentActions/cancel', id)
})
},

// Cancels a specific action by its ID.
// The action won't be retried again, but an async action cannot be aborted if its promise is stil attempting.
'chelonia.persistentActions/cancel' (id: UUIDV4): void {
if (id in this.actionsByID) {
this.actionsByID[id].cancel()
delete this.actionsByID[id]
// Likely no need to await this call.
sbp('chelonia.persistentActions/save')
}
},

// TODO: validation
'chelonia.persistentActions/configure' ({ databaseKey, options = {} }: { databaseKey: string; options: Object }): void {
this.databaseKey = databaseKey
for (const key in options) {
if (key in defaultOptions) {
defaultOptions[key] = options[key]
} else {
throw new TypeError(`${tag} Unknown option: ${key}`)
taoeffect marked this conversation as resolved.
Show resolved Hide resolved
}
}
},

'chelonia.persistentActions/enqueue' (...args): UUIDV4[] {
Copy link
Member

Choose a reason for hiding this comment

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

I think it's preferable to declare possible types of args as a comment above this function.
BTW: Why is it necessary to accept two types of arg for PersistantAction?

[invocation, options]
{ invocation, ...options }

I think the second format isn't necessary. And also invocation could be added in options of PersistantAction instance.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point about the args comment.

Why is it necessary to accept two types of arg for PersistantAction?

Here I just followed the issue requirements.

And also invocation could be added in options of PersistantAction instance.

An invocation is required in any case, so I don't think invocation should go in options since it's not actually optional.

Copy link
Member

@taoeffect taoeffect Nov 9, 2023

Choose a reason for hiding this comment

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

Here I just followed the issue requirements.

Oh, it was a good thing @Silver-IT brought this up, because I missed this in my review.

This is not what the args look like according to the issue:

[invocation, options]
{ invocation, ...options }

Rather, they look like this:

[selector, arg1, arg2, arg3]
{ invocation, ...options }

So to answer @Silver-IT's question: the reason for allowing both types is because it makes the syntax when enqueing invocations simpler.

In most cases you can just pass in the sbpInvocation like so: [selector, arg1, arg2, arg3]

But when you want to adjust the defaults for a specific invocation, you use the second syntax.

I'm noticing that this code does not correctly handle the first syntax.

EDIT: oops, my mistake, the code seems to correctly handle this.

const ids: UUIDV4[] = []
for (const arg of args) {
const action = Array.isArray(arg)
? new PersistentAction(arg)
: new PersistentAction(arg.invocation, arg)
this.actionsByID[action.id] = action
ids.push(action.id)
}
// Likely no need to await this call.
sbp('chelonia.persistentActions/save')
for (const id of ids) this.actionsByID[id].attempt()
return ids
},

// Forces retrying a given persisted action immediately, rather than waiting for the scheduled retry.
// - 'status.failedAttemptsSoFar' will still be increased upon failure.
// - Does nothing if a retry is already running.
// - Does nothing if the action has already been resolved, rejected or cancelled.
'chelonia.persistentActions/forceRetry' (id: UUIDV4): void {
if (id in this.actionsByID) {
this.actionsByID[id].attempt()
}
},

// Loads and tries every stored persistent action under the configured database key.
async 'chelonia.persistentActions/load' (): Promise<void> {
this.checkDatabaseKey()
const storedActions = JSON.parse((await sbp('chelonia/db/get', this.databaseKey)) ?? '[]')
for (const { id, invocation, options } of storedActions) {
this.actionsByID[id] = new PersistentAction(invocation, options)
// Use the stored ID instead of the autogenerated one.
// TODO: find a cleaner alternative.
this.actionsByID[id].id = id
Comment on lines +198 to +199
Copy link
Member

Choose a reason for hiding this comment

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

Why not to add id in the arguments of PersistantAction constructor?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a possible choice, but I felt like it was not that great either

}
sbp('chelonia.persistentActions/retryAll')
},

// Retry all existing persisted actions.
// TODO: add some delay between actions so as not to spam the server,
// or have a way to issue them all at once in a single network call.
'chelonia.persistentActions/retryAll' (): void {
for (const id in this.actionsByID) {
sbp('chelonia.persistentActions/forceRetry', id)
}
},

// Updates the database version of the attempting action list.
'chelonia.persistentActions/save' (): Promise<Error | void> {
this.checkDatabaseKey()
return sbp(
'chelonia/db/set',
this.databaseKey,
JSON.stringify(Object.values(this.actionsByID))
)
},

'chelonia.persistentActions/status' () {
return Object.values(this.actionsByID)
// $FlowFixMe: `PersistentAction` is incompatible with mixed
.map((action: PersistentAction) => ({ id: action.id, invocation: action.invocation, ...action.status }))
},

// Pauses every currently loaded action, and removes them from memory.
// Note: persistent storage is not affected, so that these actions can be later loaded again and retried.
'chelonia.persistentActions/unload' (): void {
for (const id in this.actionsByID) {
// Clear the action's timeout, but don't cancel it so that it can later resumed.
this.actionsByID[id].timer && clearTimeout(this.actionsByID[id].timer)
delete this.actionsByID[id]
}
}
})
Loading