Skip to content

Commit

Permalink
fix(toolkit): RWLock.acquireRead is not re-entrant (#24702)
Browse files Browse the repository at this point in the history
If multiple threads of the same process attempt to acquire the same reader lock, the a race condition occurs, and the first thread to release the reader lock will release ALL the locks.

Introduce a counter so that each acquire attempt uses a different file name, ensuring that the read lock is reentrant.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
RomainMuller authored Mar 20, 2023
1 parent 9744a82 commit 3b7431b
Showing 1 changed file with 19 additions and 7 deletions.
26 changes: 19 additions & 7 deletions packages/aws-cdk/lib/api/util/rwlock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,12 @@ import * as path from 'path';
export class RWLock {
private readonly pidString: string;
private readonly writerFile: string;
private readonly readerFile: string;
private readCounter = 0;

constructor(public readonly directory: string) {
this.pidString = `${process.pid}`;

this.writerFile = path.join(this.directory, 'synth.lock');
this.readerFile = path.join(this.directory, `read.${this.pidString}.lock`);
}

/**
Expand Down Expand Up @@ -62,14 +61,26 @@ export class RWLock {
return this.doAcquireRead();
}

/**
* Obtains the name fo a (new) `readerFile` to use. This includes a counter so
* that if multiple threads of the same PID attempt to concurrently acquire
* the same lock, they're guaranteed to use a different reader file name (only
* one thread will ever execute JS code at once, guaranteeing the readCounter
* is incremented "atomically" from the point of view of this PID.).
*/
private readerFile(): string {
return path.join(this.directory, `read.${this.pidString}.${++this.readCounter}.lock`);
}

/**
* Do the actual acquiring of a read lock.
*/
private async doAcquireRead(): Promise<ILock> {
await writeFileAtomic(this.readerFile, this.pidString);
const readerFile = this.readerFile();
await writeFileAtomic(readerFile, this.pidString);
return {
release: async () => {
await deleteFile(this.readerFile);
await deleteFile(readerFile);
},
};
}
Expand Down Expand Up @@ -102,7 +113,7 @@ export class RWLock {
* Check the current readers (if any)
*/
private async currentReaders(): Promise<number[]> {
const re = /^read\.([^.]+)\.lock$/;
const re = /^read\.([^.]+)\.[^.]+\.lock$/;
const ret = new Array<number>();

let children;
Expand Down Expand Up @@ -156,9 +167,10 @@ async function readFileIfExists(filename: string): Promise<string | undefined> {
}
}

let tmpCounter = 0;
async function writeFileAtomic(filename: string, contents: string): Promise<void> {
await fs.mkdir(path.dirname(filename), { recursive: true });
const tmpFile = `${filename}.${process.pid}`;
const tmpFile = `${filename}.${process.pid}_${++tmpCounter}`;
await fs.writeFile(tmpFile, contents, { encoding: 'utf-8' });
await fs.rename(tmpFile, filename);
}
Expand All @@ -181,4 +193,4 @@ function processExists(pid: number) {
} catch (e) {
return false;
}
}
}

0 comments on commit 3b7431b

Please sign in to comment.