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

refactor: impl Application and Agent extend with Class #65

Merged
merged 1 commit into from
Dec 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 9 additions & 2 deletions src/agent.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import { debuglog } from 'node:util';
import type { Agent, ILifecycleBoot } from 'egg';
import type { ILifecycleBoot } from 'egg';
import { WorkerStrategy } from './lib/strategy/worker.js';
import { AllStrategy } from './lib/strategy/all.js';
import { EggScheduleJobInfo } from './lib/types.js';
import type Agent from './app/extend/agent.js';

const debug = debuglog('@eggjs/schedule/agent');

Expand Down Expand Up @@ -30,7 +31,13 @@ export default class Boot implements ILifecycleBoot {

async serverDidReady(): Promise<void> {
// start schedule after worker ready
this.#agent.schedule.start();
await this.#agent.schedule.start();
debug('serverDidReady, schedule start');
}

async beforeClose(): Promise<void> {
// stop schedule before app close
await this.#agent.schedule.close();
debug('beforeClose, schedule close');
}
}
3 changes: 2 additions & 1 deletion src/app.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import { debuglog } from 'node:util';
import type {
Application, ILifecycleBoot, EggLogger,
ILifecycleBoot, EggLogger,
} from 'egg';
import type { EggScheduleJobInfo } from './lib/types.js';
import type Application from './app/extend/application.js';

const debug = debuglog('@eggjs/schedule/app');

Expand Down
27 changes: 15 additions & 12 deletions src/app/extend/agent.ts
Original file line number Diff line number Diff line change
@@ -1,30 +1,33 @@
import { Agent as EggAgent } from 'egg';
import { BaseStrategy } from '../../lib/strategy/base.js';
import { TimerStrategy } from '../../lib/strategy/timer.js';
import { Schedule } from '../../lib/schedule.js';

const SCHEDULE = Symbol('agent#schedule');
const SCHEDULE = Symbol('agent schedule');

export default {
export default class Agent extends EggAgent {
/**
* @member agent#ScheduleStrategy
*/
ScheduleStrategy: BaseStrategy,
get ScheduleStrategy() {
return BaseStrategy;
}

/**
* @member agent#TimerScheduleStrategy
*/
TimerScheduleStrategy: TimerStrategy,
get TimerScheduleStrategy() {
return TimerStrategy;
}

/**
* @member agent#schedule
*/
get schedule() {
if (!this[SCHEDULE]) {
this[SCHEDULE] = new Schedule(this);
this.lifecycle.registerBeforeClose(() => {
return this[SCHEDULE].close();
});
let schedule = this[SCHEDULE] as Schedule;
if (!schedule) {
this[SCHEDULE] = schedule = new Schedule(this);
}
return this[SCHEDULE];
},
} as any;
return schedule;
}
}
16 changes: 9 additions & 7 deletions src/app/extend/application.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,18 @@
import { Application as EggApplication } from 'egg';
import { ScheduleWorker } from '../../lib/schedule_worker.js';

const SCHEDULE_WORKER = Symbol('application#scheduleWorker');
const SCHEDULE_WORKER = Symbol('application scheduleWorker');

export default {
export default class Application extends EggApplication {
/**
* @member app#schedule
*/
get scheduleWorker() {
if (!this[SCHEDULE_WORKER]) {
this[SCHEDULE_WORKER] = new ScheduleWorker(this);
let scheduleWorker = this[SCHEDULE_WORKER] as ScheduleWorker;
if (!scheduleWorker) {
this[SCHEDULE_WORKER] = scheduleWorker = new ScheduleWorker(this);
}
return this[SCHEDULE_WORKER];
},
} as any;
return scheduleWorker;
}
}

11 changes: 5 additions & 6 deletions src/app/extend/application.unittest.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import { debuglog } from 'node:util';
import path from 'node:path';
import { importResolve } from '@eggjs/utils';
import type { ScheduleWorker } from '../../lib/schedule_worker.js';
import type { EggScheduleItem } from '../../lib/types.js';
import Application from './application.js';

const debug = debuglog('@eggjs/schedule/app');

export default {
export default class ApplicationUnittest extends Application {
async runSchedule(schedulePath: string, ...args: any[]) {
debug('[runSchedule] start schedulePath: %o, args: %o', schedulePath, args);
// for test purpose
Expand All @@ -32,10 +32,9 @@ export default {
}

debug('[runSchedule] resolve schedulePath: %o', schedulePath);
const scheduleWorker: ScheduleWorker = this.scheduleWorker;
let schedule: EggScheduleItem;
try {
schedule = scheduleWorker.scheduleItems[schedulePath];
schedule = this.scheduleWorker.scheduleItems[schedulePath];
if (!schedule) {
throw new TypeError(`Cannot find schedule ${schedulePath}`);
}
Expand All @@ -52,6 +51,6 @@ export default {
return await this.ctxStorage.run(ctx, async () => {
return await schedule.task(ctx, ...args);
});
},
} as any;
}
}

8 changes: 8 additions & 0 deletions src/index.ts
Original file line number Diff line number Diff line change
@@ -1 +1,9 @@
import Agent from './app/extend/agent.js';
import Application from './app/extend/application.js';
import ApplicationUnittest from './app/extend/application.unittest.js';

export { Agent, Application, ApplicationUnittest };
export { ScheduleWorker } from './lib/schedule_worker.js';
export { Schedule } from './lib/schedule.js';

export * from './lib/types.js';
11 changes: 6 additions & 5 deletions src/lib/schedule.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import { debuglog } from 'node:util';
import type { Agent, EggLogger } from 'egg';
import type { EggLogger } from 'egg';
import { loadSchedule } from './load_schedule.js';
import type { EggScheduleItem, EggScheduleJobInfo } from './types.js';
import type { BaseStrategy } from './strategy/base.js';
import type Agent from '../app/extend/agent.js';

const debug = debuglog('@eggjs/schedule/lib/schedule');

Expand Down Expand Up @@ -30,7 +31,7 @@ export class Schedule {
}

/**
* load all schedule jobs, then initialize and register speical strategy
* load all schedule jobs, then initialize and register special strategy
*/
async init() {
const scheduleItems = await loadSchedule(this.#agent);
Expand Down Expand Up @@ -82,18 +83,18 @@ export class Schedule {
/**
* start schedule
*/
start() {
async start() {
debug('start');
this.closed = false;
for (const instance of this.#strategyInstanceMap.values()) {
instance.start();
}
Comment on lines +86 to 91
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Critical: start() method should await instance.start()

The start() method is marked async but doesn't await the instance.start() calls. This could lead to race conditions if strategy implementations rely on async initialization.

  async start() {
    debug('start');
    this.closed = false;
    for (const instance of this.#strategyInstanceMap.values()) {
-     instance.start();
+     await instance.start();
    }
  }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
async start() {
debug('start');
this.closed = false;
for (const instance of this.#strategyInstanceMap.values()) {
instance.start();
}
async start() {
debug('start');
this.closed = false;
for (const instance of this.#strategyInstanceMap.values()) {
await instance.start();
}

}

close() {
async close() {
this.closed = true;
for (const instance of this.#strategyInstanceMap.values()) {
instance.close();
await instance.close();
}
debug('close');
Comment on lines +86 to 99
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Missing await for async start() call in Schedule class

The start() method in BaseStrategy is declared as async, but it's not being awaited in the Schedule class's start() method. This needs to be fixed for proper async/await consistency:

  • In src/lib/schedule.ts, line 90: instance.start() should be await instance.start()
🔗 Analysis chain

Verify consistent async/await usage across the codebase

Let's ensure all async method calls are properly awaited throughout the codebase.


Based on the results, I can see that instance.start() in schedule.ts is not awaited while the method is declared as async. Let me verify if the strategy instances' start methods are also async.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Find potential missing await keywords for start/close method calls
rg -A 2 "\\.(start|close)\\(\\)" --type ts

# Find all async method declarations to cross-reference
ast-grep --pattern 'async $_($$) {
  $$$
}'

Length of output: 704


Script:

#!/bin/bash
# Check if strategy instances have async start methods
ast-grep --pattern 'class $_ {
  $$$
  async start() {
    $$$
  }
  $$$
}'

# Also check for non-async start methods to be thorough
ast-grep --pattern 'class $_ {
  $$$
  start() {
    $$$
  }
  $$$
}'

Length of output: 18415

}
Expand Down
2 changes: 1 addition & 1 deletion src/lib/schedule_worker.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import type { Application } from 'egg';
import { loadSchedule } from './load_schedule.js';
import type { EggScheduleItem } from './types.js';
import type Application from '../app/extend/application.js';

export class ScheduleWorker {
#app: Application;
Expand Down
7 changes: 4 additions & 3 deletions src/lib/strategy/base.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import type { Agent, EggLogger } from 'egg';
import type { EggLogger } from 'egg';
import type { EggScheduleConfig, EggScheduleJobInfo } from '../types.js';
import type Agent from '../../app/extend/agent.js';

export class BaseStrategy {
protected agent: Agent;
Expand All @@ -21,11 +22,11 @@ export class BaseStrategy {
return this.scheduleConfig;
}

start() {
async start() {
// empty loop by default
}

close() {
async close() {
this.closed = true;
}

Expand Down
5 changes: 2 additions & 3 deletions src/lib/strategy/timer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ import cronParser from 'cron-parser';
import { ms } from 'humanize-ms';
import safeTimers from 'safe-timers';
import { logDate } from 'utility';
import type { Agent } from 'egg';
import type { EggScheduleConfig } from '../types.js';
import { BaseStrategy } from './base.js';
import type Agent from '../../app/extend/agent.js';

export abstract class TimerStrategy extends BaseStrategy {
protected cronInstance?: CronExpression;
Expand Down Expand Up @@ -34,8 +34,7 @@ export abstract class TimerStrategy extends BaseStrategy {
throw new TypeError(`[@eggjs/schedule] ${this.key} strategy should override \`handler()\` method`);
}


start() {
async start() {
/* istanbul ignore next */
if (this.agent.schedule.closed) return;

Expand Down
24 changes: 0 additions & 24 deletions src/lib/types.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
import type { ParserOptions as CronOptions } from 'cron-parser';
import type { Schedule } from './schedule.js';
import type { ScheduleWorker } from './schedule_worker.js';

/**
* Schedule Config
Expand Down Expand Up @@ -34,25 +32,3 @@ export interface EggScheduleJobInfo {
message?: string;
rt?: number;
}

declare module 'egg' {
export interface EggScheduleAgent {
schedule: Schedule;
}
export interface Agent extends EggScheduleAgent {}

export interface EggScheduleApplication {
scheduleWorker: ScheduleWorker;
/** runSchedule in unittest */
runSchedule: (schedulePath: string, ...args: any[]) => Promise<void>;
}
export interface Application extends EggScheduleApplication {}

export interface EggScheduleAppConfig {
schedule: {
directory: string[];
};
}

export interface EggAppConfig extends EggScheduleAppConfig {}
}
Loading