8000 Revert "perf(ngcc): allow immediately reporting a stale lock file (#3… · angular/angular@2be068d · GitHub
[go: up one dir, main page]
Skip to content

Commit 2be068d

Browse files
petebacondarwinalxhub
petebacondarwin
authored and
alxhub
committed
Revert "perf(ngcc): allow immediately reporting a stale lock file (#37250)" (#39435)
This reverts commit 561c0f8. The original commit provided a quick escape from an already terminal situation by killing the process if the PID in the lockfile was not found in the list of processes running on the current machine. But this broke use-cases where the node_modules was being shared between multiple machines (or more commonly Docker containers on the same actual machine). Fixes #38875 PR Close #39435
1 parent 6f3f89c commit 2be068d

File tree

2 files changed

+10
-104
lines changed

2 files changed

+10
-104
lines changed

‎packages/compiler-cli/ngcc/src/locking/async_locker.ts

Lines changed: 10 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -60,68 +60,22 @@ export class AsyncLocker {
6060
pid = newPid;
6161
}
6262
if (attempts === 0) {
63-
// Check to see if the process identified by the PID is still running. Because the
64-
// process *should* clean up after itself, we only check for a stale lock file when the
65-
// PID changes and only once. This may mean you have to wait if the process is killed
66-
// after the first check and isn't given the chance to clean up after itself.
67-
if (!this.isProcessRunning(pid)) {
68-
// try to re-lock one last time in case there was a race condition checking the process.
69-
try {
70-
return this.lockFile.write();
71-
} catch (e2) {
72-
if (e2.code !== 'EEXIST') {
73-
throw e2;
74-
}
75-
}
76-
77-
// finally check that the lock was held by the same process this whole time.
78-
const finalPid = this.lockFile.read();
79-
if (finalPid === pid) {
80-
throw new TimeoutError(this.lockFileMessage(
81-
`Lock found, but no process with PID ${pid} seems to be running.`));
82-
} else {
83-
// attempts is still 0, but adjust the PID so the message below is correct.
84-
pid = finalPid;
85-
}
86-
}
87-
88-
this.logger.info(this.lockFileMessage(
63+
this.logger.info(
8964
`Another process, with id ${pid}, is currently running ngcc.\n` +
90-
`Waiting up to ${this.retryDelay * this.retryAttempts / 1000}s for it to finish.`));
65+
`Waiting up to ${this.retryDelay * this.retryAttempts / 1000}s for it to finish.\n` +
66+
`(If you are sure no ngcc process is running then you should delete the lock-file at ${
67+
this.lockFile.path}.)`);
9168
}
9269
// The file is still locked by another process so wait for a bit and retry
9370
await new Promise(resolve => setTimeout(resolve, this.retryDelay));
9471
}
9572
}
9673
// If we fall out of the loop then we ran out of rety attempts
97-
throw new TimeoutError(this.lockFileMessage(`Timed out waiting ${
98-
this.retryAttempts * this.retryDelay /
99-
1000}s for another ngcc process, with id ${pid}, to complete.`));
100-
}
101-
102-
protected isProcessRunning(pid: string): boolean {
103-
// let the normal logic run if this is not called with a valid PID
104-
if (isNaN(+pid)) {
105-
this.logger.debug(`Cannot check if invalid PID "${pid}" is running, a number is expected.`);
106-
return true;
107-
}
108-
109-
try {
110-
process.kill(+pid, 0);
111-
return true;
112-
} catch (e) {
113-
// If the process doesn't exist ESRCH will be thrown, if the error is not that, throw it.
114-
if (e.code !== 'ESRCH') {
115-
throw e;
116-
}
117-
118-
return false;
119-
}
120-
}
121-
122-
private lockFileMessage(message: string): string {
123-
return message +
124-
`\n(If you are sure no ngcc process is running then you should delete the lock-file at ${
125-
this.lockFile.path}.)`;
74+
throw new TimeoutError(
75+
`Timed out waiting ${
76+
this.retryAttempts * this.retryDelay /
77+
1000}s for another ngcc process, with id ${pid}, to complete.\n` +
78+
`(If you are sure no ngcc process is running then you should delete the lock-file at ${
79+
this.lockFile.path}.)`);
12680
}
12781
}

‎packages/compiler-cli/ngcc/test/locking/async_locker_spec.ts

Lines changed: 0 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,6 @@ runInEachFileSystem(() => {
7171
}
7272
return lockFileContents;
7373
});
74-
spyOn(process, 'kill').and.returnValue();
7574

7675
const promise = locker.lock(async () => log.push('fn()'));
7776
// The lock is now waiting on the lock-file becoming free, so no `fn()` in the log.
@@ -81,7 +80,6 @@ runInEachFileSystem(() => {
8180
`(If you are sure no ngcc process is running then you should delete the lock-file at ${
8281
lockFile.path}.)`
8382
]]);
84-
expect(process.kill).toHaveBeenCalledWith(188, 0);
8583

8684
lockFileContents = null;
8785
// The lock-file has been removed, so we can create our own lock-file, call `fn()` and then
@@ -90,47 +88,6 @@ runInEachFileSystem(() => {
9088
expect(log).toEqual(['write()', 'read() => 188', 'write()', 'fn()', 'remove()']);
9189
});
9290

93-
it('should fail fast when waiting on a dead process', async () => {
94-
const fs = getFileSystem();
95-
const log: string[] = [];
96-
const lockFile = new MockLockFile(fs, log);
97-
const logger = new MockLogger();
98-
const locker = new AsyncLocker(lockFile, logger, 100, 10);
99-
100-
let lockFileContents: string|null = '188';
101-
spyOn(lockFile, 'write').and.callFake(() => {
102-
log.push('write()');
103-
if (lockFileContents) {
104-
throw {code: 'EEXIST'};
105-
}
106-
});
107-
spyOn(lockFile, 'read').and.callFake(() => {
108-
log.push('read() => ' + lockFileContents);
109-
if (lockFileContents === null) {
110-
throw {code: 'ENOENT'};
111-
}
112-
return lockFileContents;
113-
});
114-
spyOn(process, 'kill').and.callFake(() => {
115-
throw {code: 'ESRCH'};
116-
});
117-
118-
const promise = locker.lock(async () => log.push('fn()'));
119-
// The lock has already failed so no `fn()` in the log.
120-
expect(log).toEqual(['write()', 'read() => 188', 'write()', 'read() => 188']);
121-
expect(logger.logs.info).toEqual([]);
122-
expect(process.kill).toHaveBeenCalledWith(188, 0);
123-
// Check that a missing process errors out.
124-
let error: Error;
125-
await promise.catch(e => error = e);
126-
expect(log).toEqual(['write()', 'read() => 188', 'write()', 'read() => 188']);
127-
expect(error!.message)
128-
.toEqual(
129-
`Lock found, but no process with PID 188 seems to be running.\n` +
130-
`(If you are sure no ngcc process is running then you should delete the lock-file at ${
131-
lockFile.path}.)`);
132-
});
133-
13491
it('should extend the retry timeout if the other process locking the file changes', async () => {
13592
const fs = getFileSystem();
13693
const log: string[] = [];
@@ -152,7 +109,6 @@ runInEachFileSystem(() => {
152109
}
153110
return lockFileContents;
154111
});
155-
spyOn(process, 'kill').and.returnValue();
156112

157113
const promise = locker.lock(async () => log.push('fn()'));
158114
// The lock is now waiting on the lock-file becoming free, so no `fn()` in the log.
@@ -162,7 +118,6 @@ runInEachFileSystem(() => {
162118
`(If you are sure no ngcc process is running then you should delete the lock-file at ${
163119
lockFile.path}.)`
164120
]]);
165-
expect(process.kill).toHaveBeenCalledWith(188, 0);
166121

167122
lockFileContents = '444';
168123
// The lock-file has been taken over by another process - wait for the next attempt
@@ -176,7 +131,6 @@ runInEachFileSystem(() => {
176131
`(If you are sure no ngcc process is running then you should delete the lock-file at ${
177132
lockFile.path}.)`]
178133
]);
179-
expect(process.kill).toHaveBeenCalledWith(444, 0);
180134

181135
lockFileContents = null;
182136
// The lock-file has been removed, so we can create our own lock-file, call `fn()` and
@@ -209,13 +163,11 @@ runInEachFileSystem(() => {
209163
}
210164
return lockFileContents;
211165
});
212-
spyOn(process, 'kill').and.returnValue();
213166

214167
const promise = locker.lock(async () => log.push('fn()'));
215168

216169
// The lock is now waiting on the lock-file becoming free, so no `fn()` in the log.
217170
expect(log).toEqual(['write()', 'read() => 188']);
218-
expect(process.kill).toHaveBeenCalledWith(188, 0);
219171
// Do not remove the lock-file and let the call to `lock()` timeout.
220172
let error: Error;
221173
await promise.catch(e => error = e);

0 commit comments

Comments
 (0)
0