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 async tasks #739

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Add async tasks #739

wants to merge 2 commits into from

Conversation

joshbwlng
Copy link
Contributor

@joshbwlng joshbwlng commented Feb 6, 2024

Change-type: minor


Spec: https://balena.fibery.io/Inputs/Research/PineJS-Async-Tasks-483

Testing locally:

docker-compose -f docker-compose.npm-test.yml up -d
export DATABASE_URL=postgres://docker:docker@localhost:5431/postgres
export PINEJS_QUEUE_CONCURRENCY=1
TZ=UTC npm run mocha test/08-tasks.test.ts

@joshbwlng joshbwlng self-assigned this Feb 6, 2024
@joshbwlng joshbwlng mentioned this pull request Feb 6, 2024
@joshbwlng joshbwlng force-pushed the joshbwlng/tasks branch 11 times, most recently from daecf95 to 48d91f4 Compare February 10, 2024 03:39
@joshbwlng joshbwlng force-pushed the joshbwlng/tasks branch 2 times, most recently from 0863e32 to 0152c12 Compare February 22, 2024 03:24
@joshbwlng joshbwlng force-pushed the joshbwlng/tasks branch 8 times, most recently from 11d7ea0 to 24eda29 Compare March 6, 2024 02:17
import { sbvrUtils } from '../../../src/server-glue/module';

// Define JSON schema for accepted parameters
const createDeviceParamsSchema = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: I also investigated using zod to define parameter object shapes, but decided to stick with JSON Schema as its a commonly used standard. Consumers can still define their shapes with zod and then convert to JSON Schema using something like zod-to-json-schema when defining task handlers.

@joshbwlng joshbwlng force-pushed the joshbwlng/tasks branch 2 times, most recently from 9e1fe55 to a83c83b Compare April 15, 2024 01:27
@joshbwlng joshbwlng force-pushed the joshbwlng/tasks branch 2 times, most recently from 33d3587 to 56c035f Compare April 25, 2024 23:06
src/tasks/worker.ts Outdated Show resolved Hide resolved
@joshbwlng joshbwlng force-pushed the joshbwlng/tasks branch 5 times, most recently from 4a165ae to f9c7bcd Compare April 30, 2024 23:13
@joshbwlng joshbwlng requested review from Page- and thgreasi May 5, 2024 05:34
@joshbwlng joshbwlng marked this pull request as ready for review May 8, 2024 22:31
@flowzone-app flowzone-app bot enabled auto-merge May 8, 2024 22:42

// Start listening and polling for tasks
public start(): void {
sbvrUtils.db.on?.(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is the ?. needed? I feel like if it's ever undefined then the entire task system would break and that's not something we'd want to silently ignore

if (this.canExecute()) {
await sbvrUtils.db.transaction(async (tx) => {
const result = await tx.executeSql(
`SELECT ${selectColumns} FROM task AS t WHERE id = $1 FOR UPDATE SKIP LOCKED`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this compliant with postgres+mysql+sqlite? If not we should have a warning if you're trying to use with an unsupported db engine

Comment on lines +197 to +199
if (!this.canExecute()) {
return;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be outside of the transaction imo to avoid the unnecessary db interaction. Presumably you've put it inside to be in a promise chain but I think it'd be better to do something like

(async () => {
	try {
		let executed = false;
		const handlerNames = Object.keys(this.handlers);
		const binds = handlerNames.map((_, index) => `$${index + 1}`).join(', ');
		if (!this.canExecute()) {
			return;
		}
		await sbvrUtils.db.transaction(async (tx) => {
			...
		});
	} catch (err) {
		...
	} finally {
		...
	}
})();

to be able to use async/await as usual and put the try/catch/finally around everything


// Calculate next attempt time using exponential backoff
private getNextAttemptTime(attempt: number): Date | null {
const delay = Math.ceil(Math.exp(Math.min(10, attempt))) * 1000;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm pretty sure this is not doing what you expect as by my calculations this would be setting the minimum delay to a bit over 6 hours (e^10)? I think it probably makes sense to use logic more similar to https://github.com/balena-io-modules/pinejs-client-js/blob/v6.14.6/src/index.ts#L1123 for the next delay time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Page- My mistake here was * 1000, not sure how I missed it. I didn't come up with the formula myself, I pulled it from graphile/worker: https://worker.graphile.org/docs/exponential-backoff

Comment on lines +170 to +171
},
body: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
},
body: {
},
options: {
returnResource: false,
},
body: {

Comment on lines 70 to 74
// Create trigger function if it doesn't exist
await createTrigger(tx);

// Create indexes if they don't exist
await createIndexes(tx);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be able to be done by defining and initSql in the model definition the same way as user models would

Comment on lines 162 to 164
if (worker == null) {
return;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume this is because we only support tasks on postgres, which is fine but should throw an error here rather than silently/surprisingly doing nothing

modelText,
customServerCode: exports,
},
] as sbvrUtils.ExecutableModel[],
Copy link
Collaborator

Choose a reason for hiding this comment

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

satisfies is better because it doesn't cast in the same way

Suggested change
] as sbvrUtils.ExecutableModel[],
] satisfies sbvrUtils.ExecutableModel[],

although I think it'd be better to use

export const config ConfigLoader.Config = {

so that the whole thing is typed and matches the expectation

export const apiRoot = 'tasks';

// Channel name for task insert notifications
export const channel = 'task_insert';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be something like pinejs$task_insert to try to namespace it more?

Comment on lines 705 to 709
client.on(name, (msg) => {
fn(msg).catch((error) => {
console.error('Error handling message:', error);
});
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer try/catch with await so we can handle both sync/async exceptions, just as additional safety

Suggested change
client.on(name, (msg) => {
fn(msg).catch((error) => {
console.error('Error handling message:', error);
});
});
client.on(name, async (msg) => {
try {
await fn(msg);
} catch (error) {
console.error('Error handling message:', error);
}
});

Change-type: minor
@joshbwlng joshbwlng force-pushed the joshbwlng/tasks branch 2 times, most recently from 8b3bb22 to 5073fad Compare June 9, 2024 23:04
@joshbwlng joshbwlng marked this pull request as draft June 10, 2024 02:31
auto-merge was automatically disabled June 10, 2024 02:31

Pull request was converted to draft

@joshbwlng joshbwlng force-pushed the joshbwlng/tasks branch 2 times, most recently from 6d57027 to 1c3c1de Compare June 10, 2024 04:15
Change-type: patch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants