Skip to content

Commit

Permalink
fix(pacmak): race condition in python packing when run over multiple …
Browse files Browse the repository at this point in the history
…packages (#1783)

The root cause seems to be that the async APIs in Node's fs modules
isn't strictly single threaded.

When packed over several packages, pacmak schedules these in parallel,
creating a race condition on the local cache for Python Black.
All threads thrash to try and set up the package directory and one of
them fails.

The fix is to introduce a process wide promise that awaits until the
Python Black is fully set up.

Testing
Unit tests for such race conditions are hard to write correctly. This
has been verified manually by running against 10 CDK packages at a time.
I was able to confirm that the issue does show up consistently without
this fix and the fix works correctly.

---

By submitting this pull request, I confirm that my contribution is made under the terms of the [Apache 2.0 license].

[Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0
  • Loading branch information
Niranjan Jayakar authored Jul 13, 2020
1 parent d83e004 commit 7807027
Showing 1 changed file with 8 additions and 1 deletion.
9 changes: 8 additions & 1 deletion packages/jsii-pacmak/lib/targets/python.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ import { die, toPythonIdentifier } from './python/util';
const spdxLicenseList = require('spdx-license-list');

export default class Python extends Target {
private static BLACK_PATH?: Promise<string>;

protected readonly generator: PythonGenerator;

public constructor(options: TargetOptions) {
Expand All @@ -45,8 +47,13 @@ export default class Python extends Target {
public async generateCode(outDir: string, tarball: string): Promise<void> {
await super.generateCode(outDir, tarball);

// Using a static variable as a lock to prevent racing. Since blackPath() uses
// Promise APIs from fs and os modules (that use libuv), an additional lock is required.
if (Python.BLACK_PATH === undefined) {
Python.BLACK_PATH = this.blackPath();
}
// We'll just run "black" on that now, to make the generated code a little more readable.
await shell(await this.blackPath(), ['--py36', outDir], {
await shell(await Python.BLACK_PATH, ['--py36', outDir], {
cwd: outDir,
});
}
Expand Down

0 comments on commit 7807027

Please sign in to comment.