diff --git a/lib/services/scheduled-jobs.service.ts b/lib/services/scheduled-jobs.service.ts index 1c000bd..af3a923 100644 --- a/lib/services/scheduled-jobs.service.ts +++ b/lib/services/scheduled-jobs.service.ts @@ -83,8 +83,11 @@ export class ScheduledJobs implements OnModuleInit, OnModuleDestroy { ) {} onModuleInit() { - /** Which job each key came from: two different jobs may share a key only if both set it. */ - const owners = new Map(); + /** + * Which job each key came from: two different jobs (another method, or the same one on + * another instance) may share a key only if both set it. + */ + const owners = new Map(); /** The jobs on each key, for the collision check against the other keys and the elections. */ const jobs = new Map(); @@ -98,27 +101,33 @@ export class ScheduledJobs implements OnModuleInit, OnModuleDestroy { if (!definition) { continue; } + // Named after the instance's class, not the one declaring the method: each subclass + // that inherits a decorated method is a job of its own. + const job = describeJob(definition, instance); if (!wrapper.isDependencyTreeStatic()) { this.logger.warn( - `${describeJob(definition)} is decorated with ${decoratorsOf(definition)}, but ${wrapper.name as string} is ` + + `${job} is decorated with ${decoratorsOf(definition)}, but ${wrapper.name as string} is ` + 'not a singleton provider (it is request-scoped or transient, or depends on one), so its calls will fail.', ); continue; } - const key = definition.key ?? definition.derivedKey; + const key = definition.key ?? job; const previous = owners.get(key); - if (previous && previous !== definition && (previous.key === undefined || definition.key === undefined)) { + if (previous && previous.instance === instance && previous.definition === definition) { + continue; // the same provider under another token (`useExisting`) + } + if (previous && (previous.definition.key === undefined || definition.key === undefined)) { throw new Error( - `LocksModule: ${describeJob(previous)} and ${describeJob(definition)} both use the lock key "${key}". ` + + `LocksModule: ${previous.job} and ${job} both use the lock key "${key}". ` + 'Two jobs share a key only when both set it on purpose (to exclude each other); set `key` on ' + - `${decoratorsOf(definition.key === undefined ? definition : previous)} to tell them apart.`, + `${decoratorsOf(definition.key === undefined ? definition : previous.definition)} to tell them apart.`, ); } - owners.set(key, definition); - if (!jobs.get(key)?.includes(describeJob(definition))) { - jobs.set(key, [...(jobs.get(key) ?? []), describeJob(definition)]); + owners.set(key, { instance, definition, job }); + if (!jobs.get(key)?.includes(job)) { + jobs.set(key, [...(jobs.get(key) ?? []), job]); } - this.register(instance, definition, key); + this.register(instance, definition, key, job); } } // A job holds `key` (its run lock) and `key:owner` (its lease): neither may be another @@ -154,13 +163,13 @@ export class ScheduledJobs implements OnModuleInit, OnModuleDestroy { ); } - private register(instance: object, definition: JobDefinition, key: string) { + private register(instance: object, definition: JobDefinition, key: string, job: string) { const internals = this.locks[LOCKS_INTERNALS]; let state = this.states.get(key); if (!state) { this.states.set(key, (state = new KeyState(key, internals, this.logger))); } - const runner = this.runner(definition, state, internals); + const runner = this.runner(definition, job, state, internals); let runners = jobRunners.get(instance); if (!runners) { jobRunners.set(instance, (runners = new Map())); @@ -168,8 +177,7 @@ export class ScheduledJobs implements OnModuleInit, OnModuleDestroy { runners.set(definition, runner); } - private runner(definition: JobDefinition, state: KeyState, internals: LocksInternals): JobRunner { - const job = describeJob(definition); + private runner(definition: JobDefinition, job: string, state: KeyState, internals: LocksInternals): JobRunner { const skip = (why: string) => { this.logger.debug(`Skipped ${job}: ${why}`); return undefined; diff --git a/lib/utils/job-decorator.util.ts b/lib/utils/job-decorator.util.ts index f5a9d6a..df5f179 100644 --- a/lib/utils/job-decorator.util.ts +++ b/lib/utils/job-decorator.util.ts @@ -1,6 +1,6 @@ import { JOB_DEFINITION } from '../locks.constants.js'; import type { OnOneInstanceOptions, WithoutOverlappingOptions } from '../interfaces/job-options.interface.js'; -import { decoratorsOf, describeJob, jobRunners, type JobDefinition } from './job-registry.util.js'; +import { decoratorsOf, describeJob, jobClassName, jobRunners, type JobDefinition } from './job-registry.util.js'; import { ttlMs } from './ttl.util.js'; type Kind = 'oneInstance' | 'withoutOverlapping'; @@ -64,15 +64,14 @@ function wrap(method: (...args: unknown[]) => unknown, target: object, propertyK const definition: JobDefinition = { className: target.constructor.name, methodName: String(propertyKey), - derivedKey: `${target.constructor.name}.${String(propertyKey)}`, }; const wrapper: Wrapped = async function (this: object, ...args: unknown[]) { const runner = jobRunners.get(this)?.get(definition); if (!runner) { throw new Error( - `${describeJob(definition)} is decorated with ${decoratorsOf(definition)}, but LocksModule doesn't know this ` + - `instance, so it can't tell whether this call may run. Import LocksModule in the application, and provide ` + - `${definition.className} as a singleton provider (not request-scoped or transient, not created with \`new\`).`, + `${describeJob(definition, this)} is decorated with ${decoratorsOf(definition)}, but LocksModule doesn't know ` + + `this instance, so it can't tell whether this call may run. Import LocksModule in the application, and provide ` + + `${jobClassName(definition, this)} as a singleton provider (not request-scoped or transient, not created with \`new\`).`, ); } diff --git a/lib/utils/job-registry.util.ts b/lib/utils/job-registry.util.ts index ed5e65c..6a9d288 100644 --- a/lib/utils/job-registry.util.ts +++ b/lib/utils/job-registry.util.ts @@ -1,9 +1,8 @@ /** What `@OnOneInstance()` and `@WithoutOverlapping()` record on the method they wrap. */ export interface JobDefinition { + /** The class that declares the method: its subclasses inherit the job, each with a key of its own. */ className: string; methodName: string; - /** `ClassName.methodName`: the key when neither decorator sets one. */ - derivedKey: string; /** Set by either decorator. */ key?: string; /** `@OnOneInstance()`: the ownership lease's ttl in ms, or the module's. */ @@ -24,8 +23,17 @@ export interface JobRunner { */ export const jobRunners = new WeakMap>(); -export function describeJob(definition: JobDefinition): string { - return `${definition.className}.${definition.methodName}`; +/** + * `ClassName.methodName`, named after the instance's class when given one: the key when neither + * decorator sets one. A subclass that inherits a decorated method is a job of its own. + */ +export function describeJob(definition: JobDefinition, instance?: object): string { + return `${jobClassName(definition, instance)}.${definition.methodName}`; +} + +/** The instance's class when given one, else the class that declares the method. */ +export function jobClassName(definition: JobDefinition, instance?: object): string { + return (instance && (instance.constructor as { name?: string } | undefined)?.name) || definition.className; } export function decoratorsOf(definition: JobDefinition): string { diff --git a/tests/scheduling.spec.ts b/tests/scheduling.spec.ts index 11b82f5..6b157a2 100644 --- a/tests/scheduling.spec.ts +++ b/tests/scheduling.spec.ts @@ -403,6 +403,52 @@ describe('@OnOneInstance() and @WithoutOverlapping()', () => { await exporting; }); + it('gives each subclass that inherits a decorated method a key of its own', async () => { + abstract class ReportJob { + @OnOneInstance() + @WithoutOverlapping() + async run() { + runs.push({ job: this.constructor.name, instance: 'a' }); + await gate; + } + } + @Injectable() + class SalesReport extends ReportJob {} + @Injectable() + class StockReport extends ReportJob {} + const app = await startInstance({ providers: [SalesReport, StockReport], store, locks: { clock }, logger }); + apps.push(app); + const hold = deferred(); + gate = hold.promise; + const sales = app.get(SalesReport).run(); + const stock = app.get(StockReport).run(); + await until(() => runs.length === 2); // neither excludes the other + expect(await app.get(SalesReport).run()).toBeUndefined(); // but each still excludes itself + expect(logger.matching('DEBUG Skipped SalesReport.run: a run is still in progress')).toHaveLength(1); + hold.resolve(); + await Promise.all([sales, stock]); + expect(runs.map((r) => r.job).sort()).toEqual(['SalesReport', 'StockReport']); + expect(logger.matching(/runs on this instance \(lease "(Sales|Stock)Report\.run:owner"/)).toHaveLength(2); + }); + + it('refuses one job class provided twice under its default key, but not an alias of the same instance', async () => { + @Injectable() + class Sync { + @WithoutOverlapping() + run() {} + } + const twice = await Test.createTestingModule({ + imports: [LocksModule.forRoot()], + providers: [Sync, { provide: 'second', useClass: Sync }], + }).compile(); + twice.useLogger(false); + await expect(twice.init()).rejects.toThrow('LocksModule: Sync.run and Sync.run both use the lock key "Sync.run".'); + + const app = await startInstance({ providers: [Sync, { provide: 'alias', useExisting: Sync }], store, locks: { clock }, logger }); + apps.push(app); + expect(app.get('alias')).toBe(app.get(Sync)); + }); + it("refuses a job whose key is another job's lease (`:owner`)", async () => { @Injectable() class Nightly {