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.

Sign up for GitHub

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

chore: Refactor cronjob controller #3029

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

chore: Refactor cronjob controller #3029

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
f2d8bed
refactor cronjob controller
hmalik88 Jan 21, 2025
75020fb
undo comment deletion
hmalik88 Jan 21, 2025
bd7c7c2
Merge branch 'main' into hm/refactor-cronjob-controller
hmalik88 Jan 21, 2025
33dfc92
fix coverage
hmalik88 Jan 21, 2025
546f791
fix coverage again
hmalik88 Jan 21, 2025
5c7ae9a
Merge branch 'main' into hm/refactor-cronjob-controller
hmalik88 Jan 23, 2025
9452deb
apply code review
hmalik88 Jan 23, 2025
2d380a2
update coverage
hmalik88 Jan 23, 2025
131a290
revert change to destroy method
hmalik88 Jan 23, 2025
f7f4cb5
update coverage
hmalik88 Jan 23, 2025
eafee0f
Merge branch 'main' into hm/refactor-cronjob-controller
hmalik88 Jan 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions packages/snaps-controllers/coverage.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"branches": 92.96,
"functions": 96.56,
"lines": 98.05,
"statements": 97.77
"branches": 93.01,
"functions": 97.15,
"lines": 98.15,
"statements": 97.87
}
2 changes: 1 addition & 1 deletion packages/snaps-controllers/src/cronjob/CronjobController.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,7 @@ describe('CronjobController', () => {

expect(() =>
cronjobController.scheduleBackgroundEvent(backgroundEvent),
).toThrow('Cannot schedule an event in the past.');
).toThrow('Cannot schedule execution in the past.');

expect(cronjobController.state.events).toStrictEqual({});

Expand Down
247 changes: 143 additions & 104 deletions packages/snaps-controllers/src/cronjob/CronjobController.ts
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

I haven't reviewed this in detail. but generally when I proposed we refactor the CronjobController, the main goal I had in mind was to unify the logic used to manage background events and cronjobs.

We currently treat these as entirely separate entities, but I don't think they need to be. My general thought was that we should investigate treating them as one entity with the only variation being non-recurring versus recurring cronjobs (or background events - no strong opinion on the name).

I had intended for the referenced ticket to be to investigate ways to achieve this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR unifies some logic, but I inherently found it hard to make a mental model of both being the same thing because the term non-recurring cronjob in of itself is an oxymoron. Maybe there's a better term to encompass both things. One divergence is that we fetch the cron jobs every time from the manifest, maybe if we can fetch once and unify the state then some of the other logic itself can be refactored. We can keep the original ticket open, I think this is an improvement nonetheless.

Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,11 @@
| SnapDisabled
| CronjobControllerStateChangeEvent;

type BackgroundEventActions =
| `${typeof controllerName}:scheduleBackgroundEvent`
| `${typeof controllerName}:cancelBackgroundEvent`
| `${typeof controllerName}:getBackgroundEvents`;

export type CronjobControllerMessenger = RestrictedControllerMessenger<
typeof controllerName,
CronjobControllerActions,
Expand Down Expand Up @@ -152,55 +157,128 @@
this._handleEventSnapUpdated = this._handleEventSnapUpdated.bind(this);
this._handleSnapDisabledEvent = this._handleSnapDisabledEvent.bind(this);
this._handleSnapEnabledEvent = this._handleSnapEnabledEvent.bind(this);

// Subscribe to Snap events
/* eslint-disable @typescript-eslint/unbound-method */
this.#initializeEventListeners();

this.messagingSystem.subscribe(
'SnapController:snapInstalled',
this._handleSnapRegisterEvent,
);
// Register action handlers
this.#initializeActionHandlers();

this.messagingSystem.subscribe(
'SnapController:snapUninstalled',
this._handleSnapUnregisterEvent,
);
this.dailyCheckIn().catch((error) => {
logError(error);
});

this.messagingSystem.subscribe(
'SnapController:snapEnabled',
this._handleSnapEnabledEvent,
);
this.#rescheduleBackgroundEvents(Object.values(this.state.events));
}

this.messagingSystem.subscribe(
'SnapController:snapDisabled',
this._handleSnapDisabledEvent,
);
/**
* Initialize event listeners.
*/
#initializeEventListeners() {
/* eslint-disable @typescript-eslint/unbound-method */
const events = {
'SnapController:snapInstalled': this._handleSnapRegisterEvent,
'SnapController:snapUninstalled': this._handleSnapUnregisterEvent,
'SnapController:snapEnabled': this._handleSnapEnabledEvent,
'SnapController:snapDisabled': this._handleSnapDisabledEvent,
'SnapController:snapUpdated': this._handleEventSnapUpdated,
};
/* eslint-disable @typescript-eslint/unbound-method */

this.messagingSystem.subscribe(
'SnapController:snapUpdated',
this._handleEventSnapUpdated,
);
/* eslint-enable @typescript-eslint/unbound-method */
Object.entries(events).forEach(([event, handler]) => {
this.messagingSystem.subscribe(
event as CronjobControllerEvents['type'],
handler,
);
});
}

this.messagingSystem.registerActionHandler(
`${controllerName}:scheduleBackgroundEvent`,
(...args) => this.scheduleBackgroundEvent(...args),
);
/**
* Initialize action handlers.
*/
#initializeActionHandlers(): void {
const handlers: Record<BackgroundEventActions, (...args: any[]) => any> = {
[`${controllerName}:scheduleBackgroundEvent`]: (
event: Omit<BackgroundEvent, 'id' | 'scheduledAt'>,
) => this.scheduleBackgroundEvent(event),
[`${controllerName}:cancelBackgroundEvent`]: (
origin: string,
id: string,
) => this.cancelBackgroundEvent(origin, id),
[`${controllerName}:getBackgroundEvents`]: (snapId: SnapId) =>
this.getBackgroundEvents(snapId),
};

this.messagingSystem.registerActionHandler(
`${controllerName}:cancelBackgroundEvent`,
(...args) => this.cancelBackgroundEvent(...args),
);
Object.entries(handlers).forEach(([action, handler]) => {
this.messagingSystem.registerActionHandler(
action as BackgroundEventActions,
handler,
);
});
}
hmalik88 marked this conversation as resolved.
Show resolved Hide resolved

this.messagingSystem.registerActionHandler(
`${controllerName}:getBackgroundEvents`,
(...args) => this.getBackgroundEvents(...args),
);
/**
* Execute a request.
*
* @param snapId - ID of a Snap.
* @param request - Request to be executed.
*/
async #executeRequest(snapId: SnapId, request: unknown) {
await this.messagingSystem
.call('SnapController:handleRequest', {
snapId,
origin: '',
handler: HandlerType.OnCronjob,
request: request as Record<string, unknown>,
})
.catch((error) => {
logError(error);

Check warning on line 235 in packages/snaps-controllers/src/cronjob/CronjobController.ts

View check run for this annotation

Codecov / codecov/patch

packages/snaps-controllers/src/cronjob/CronjobController.ts#L234-L235 

Added lines #L234 - L235 were not covered by tests
});
}

this.dailyCheckIn().catch((error) => {
logError(error);
/**
* Setup a timer.
*
* @param id - ID of a timer.
* @param ms - Time in milliseconds.
* @param snapId - ID of a Snap.
* @param onComplete - Callback function to be executed when the timer completes.
* @param validateTime - Whether the time should be validated.
*/
#setupTimer(
id: string,
ms: number,
snapId: SnapId,
onComplete: () => void,
validateTime = true,
) {
if (validateTime && ms <= 0) {
throw new Error('Cannot schedule execution in the past.');
}

const timer = new Timer(ms);
timer.start(() => {
onComplete();
this.#timers.delete(id);
this.#snapIds.delete(id);
});

this.#rescheduleBackgroundEvents(Object.values(this.state.events));
this.#timers.set(id, timer);
this.#snapIds.set(id, snapId);
}

/**
* Cleanup a timer.
*
* @param id - ID of a timer.
*/
#cleanupTimer(id: string) {
const timer = this.#timers.get(id);
if (timer) {
timer.cancel();
this.#timers.delete(id);
this.#snapIds.delete(id);
}
}

/**
Expand Down Expand Up @@ -274,23 +352,21 @@
return;
}

const timer = new Timer(ms);
timer.start(() => {
this.#executeCronjob(job).catch((error) => {
this.#setupTimer(
job.id,
ms,
job.snapId,
() => {
// TODO: Decide how to handle errors.
logError(error);
});

this.#timers.delete(job.id);
this.#schedule(job);
});
this.#executeCronjob(job).catch(logError);
this.#schedule(job);
},
false,
);

if (!this.state.jobs[job.id]?.lastRun) {
this.#updateJobLastRunState(job.id, 0); // 0 for init, never ran actually
}

this.#timers.set(job.id, timer);
this.#snapIds.set(job.id, job.snapId);
}

/**
Expand All @@ -300,12 +376,7 @@
*/
async #executeCronjob(job: Cronjob) {
this.#updateJobLastRunState(job.id, Date.now());
await this.messagingSystem.call('SnapController:handleRequest', {
snapId: job.snapId,
origin: '',
handler: HandlerType.OnCronjob,
request: job.request,
});
await this.#executeRequest(job.snapId, job.request);
}

/**
Expand Down Expand Up @@ -359,10 +430,7 @@
'Only the origin that scheduled this event can cancel it.',
);

const timer = this.#timers.get(id);
timer?.cancel();
this.#timers.delete(id);
this.#snapIds.delete(id);
this.#cleanupTimer(id);
this.update((state) => {
delete state.events[id];
});
Expand All @@ -378,32 +446,12 @@
const now = new Date();
const ms = date.getTime() - now.getTime();

if (ms <= 0) {
throw new Error('Cannot schedule an event in the past.');
}

const timer = new Timer(ms);
timer.start(() => {
this.messagingSystem
.call('SnapController:handleRequest', {
snapId: event.snapId,
origin: '',
handler: HandlerType.OnCronjob,
request: event.request,
})
.catch((error) => {
logError(error);
});

this.#timers.delete(event.id);
this.#snapIds.delete(event.id);
this.#setupTimer(event.id, ms, event.snapId, () => {
this.#executeRequest(event.snapId, event.request).catch(logError);
this.update((state) => {
delete state.events[event.id];
});
});

this.#timers.set(event.id, timer);
this.#snapIds.set(event.id, event.snapId);
}

/**
Expand Down Expand Up @@ -532,32 +580,23 @@
super.destroy();

/* eslint-disable @typescript-eslint/unbound-method */
this.messagingSystem.unsubscribe(
'SnapController:snapInstalled',
this._handleSnapRegisterEvent,
);

this.messagingSystem.unsubscribe(
'SnapController:snapUninstalled',
this._handleSnapUnregisterEvent,
);

this.messagingSystem.unsubscribe(
'SnapController:snapEnabled',
this._handleSnapEnabledEvent,
);

this.messagingSystem.unsubscribe(
'SnapController:snapDisabled',
this._handleSnapDisabledEvent,
);
const events = {
'SnapController:snapInstalled': this._handleSnapRegisterEvent,
'SnapController:snapUninstalled': this._handleSnapUnregisterEvent,
'SnapController:snapEnabled': this._handleSnapEnabledEvent,
'SnapController:snapDisabled': this._handleSnapDisabledEvent,
'SnapController:snapUpdated': this._handleEventSnapUpdated,
};

this.messagingSystem.unsubscribe(
'SnapController:snapUpdated',
this._handleEventSnapUpdated,
);
/* eslint-enable @typescript-eslint/unbound-method */

Object.entries(events).forEach(([event, handler]) => {
this.messagingSystem.unsubscribe(
event as CronjobControllerEvents['type'],
handler,
);
});

this.#snapIds.forEach((snapId) => this.unregister(snapId));
}

Expand Down
Loading