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
Prev Previous commit
Next Next commit
Emit events on action success and failure
  • Loading branch information
snowteamer committed Oct 30, 2023
commit 6192b3a69c1095035f111ca58b80579264f7910d
3 changes: 3 additions & 0 deletions shared/domains/chelonia/events.js
Original file line number Diff line number Diff line change
@@ -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'
126 changes: 71 additions & 55 deletions shared/domains/chelonia/persistent-actions.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
'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,
@@ -11,91 +14,98 @@ type PersistentActionOptions = {
// How many seconds to wait between retries.
retrySeconds: number,
skipCondition?: SbpInvocation,
// SBP selector to call on success with the received value.
successInvocationSelector?: string,
totalFailureInvocation?: SbpInvocation
}

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

const defaultOptions: PersistentActionOptions = {
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 = {
cancelled: false,
attempting: false,
failedAttemptsSoFar: 0,
lastError: '',
nextRetry: '',
pending: false
resolved: false
}
}

// Do not call if the action is pending or cancelled!
async attempt (): Promise<void> {
// Bail out if the action is already attempting or resolved.
if (this.status.attempting || this.status.resolved) return
if (await this.trySBP(this.options.skipCondition)) {
this.cancel()
return
}
try {
this.status.pending = true
this.status.attempting = true
const result = await sbp(...this.invocation)
this.status.attempting = false
this.handleSuccess(result)
} catch (error) {
this.status.attempting = false
this.handleError(error)
}
}

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

async handleError (error: Error): Promise<void> {
const { options, status } = this
// Update relevant status fields before calling any optional selector.
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
status.nextRetry = anyAttemptLeft && !status.cancelled
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.
sbp('okTurtles.events/emit', PERSISTENT_ACTION_FAILURE, { error, id })
await this.trySBP(options.errorInvocation)
!anyAttemptLeft && await this.trySBP(options.totalFailureInvocation)
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

}
status.pending = false
}

async handleSuccess (result: any): Promise<void> {
const { status } = this
handleSuccess (result: any): void {
const { id, status } = this
status.lastError = ''
status.nextRetry = ''
status.pending = false
this.options.successInvocationSelector &&
await this.trySBP([this.options.successInvocationSelector, result])
status.resolved = true
sbp('okTurtles.events/emit', PERSISTENT_ACTION_SUCCESS, { id, result })
}

async trySBP (invocation: SbpInvocation | void): Promise<any> {
@@ -112,14 +122,19 @@ class PersistentAction {
sbp('sbp/selectors/register', {
'chelonia.persistentActions/_init' (): void {
this.actionsByID = Object.create(null)
this.nextID = 0
// Necessary for now as _init cannot be async. Becomes true when 'configure' has been called.
this.ready = false
Copy link
Member

Choose a reason for hiding this comment

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

I can't find the place to set this.ready true in configure function. Did you miss it by mistake?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, good catch. BTW I've since replaced it by a direct check for availability of databaseKey

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 pending.
'chelonia.persistentActions/cancel' (id: number): void {
// 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]
@@ -128,19 +143,28 @@ sbp('sbp/selectors/register', {
}
},

'chelonia.persistentActions/configure' (options: { databaseKey: string }): void {
this.databaseKey = options.databaseKey
// TODO: validation
'chelonia.persistentActions/configure' ({ databaseKey, options = {} }: { databaseKey: string; options: Object }): void {
if (!databaseKey) throw new TypeError(`${tag} 'databaseKey' is required`)
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): number[] {
'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.

if (!this.ready) throw new Error(`${tag} Not ready yet.`)
const ids: number[] = []
const ids: UUIDV4[] = []
for (const arg of args) {
const id = this.nextID++
this.actionsByID[id] = Array.isArray(arg)
const action = Array.isArray(arg)
? new PersistentAction(arg)
: new PersistentAction(arg.invocation, arg)
ids.push(id)
this.actionsByID[action.id] = action
ids.push(action.id)
}
// Likely no need to await this call.
sbp('chelonia.persistentActions/save')
@@ -150,29 +174,21 @@ sbp('sbp/selectors/register', {

// Forces retrying an existing persisted action given its ID.
// Note: 'failedAttemptsSoFar' will still be increased upon failure.
async 'chelonia.persistentActions/forceRetry' (id: number): Promise<void> {
'chelonia.persistentActions/forceRetry' (id: UUIDV4): void {
if (id in this.actionsByID) {
const action = this.actionsByID[id]
// Bail out if the action is already pending or cancelled.
if (action.status.pending || action.status.cancelled) return
try {
await action.attempt()
// If the action succeded, delete it and update the DB.
delete this.actionsByID[id]
sbp('chelonia.persistentActions/save')
} catch {
// Do nothing.
}
this.actionsByID[id].attempt()
}
},

// Called on login to load the correct set of actions for the current user.
// Loads and tries every stored persistent action under the configured database key.
async 'chelonia.persistentActions/load' (): Promise<void> {
const { actionsByID = {}, nextID = 0 } = (await sbp('chelonia/db/get', this.databaseKey)) ?? {}
for (const id in actionsByID) {
this.actionsByID[id] = new PersistentAction(actionsByID[id].invocation, actionsByID[id].options)
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

}
this.nextID = nextID
this.ready = true
sbp('chelonia.persistentActions/retryAll')
},
@@ -186,18 +202,18 @@ sbp('sbp/selectors/register', {
}
},

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

'chelonia.persistentActions/status' (id: number) {
if (id in this.actionsByID) {
return JSON.stringify(this.actionsByID[id])
}
'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 }))
}
})