Skip to content

Commit

Permalink
Prevent reads from this.process when this.process can be async updated.
Browse files Browse the repository at this point in the history
Only synchronized methods can write to this.process, and we don't want to synchronize too much.

Also fixes the isAlive method in FakeSubprocess.

RELNOTES: n/a
PiperOrigin-RevId: 341587731
  • Loading branch information
larsrc-google authored and copybara-github committed Nov 10, 2020
1 parent a9bf093 commit 780e27d
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,13 @@ void resetResponseChecker(Integer requestId) throws InterruptedException {
* {@code workerProcessResponse} and release the semaphore for the {@code WorkerProxy}.
*/
private void waitResponse() throws InterruptedException, IOException {
recordingStream = new RecordingInputStream(process.getInputStream());
Subprocess p = this.process;
if (p == null || !p.isAlive()) {
// Avoid busy-wait for a new process.
Thread.sleep(1);
return;
}
recordingStream = new RecordingInputStream(p.getInputStream());
recordingStream.startRecording(4096);
WorkResponse parsedResponse = WorkResponse.parseDelimitedFrom(recordingStream);

Expand Down Expand Up @@ -303,14 +309,14 @@ String getRecordingStreamMessage() {

/** Returns true if this process has died for other reasons than a call to {@code #destroy()}. */
boolean diedUnexpectedly() {
return process != null && !process.isAlive() && !isInterrupted;
Subprocess p = this.process; // Protects against this.process getting null.
return p != null && !p.isAlive() && !isInterrupted;
}

/** Returns the exit value of multiplexer's process, if it has exited. */
Optional<Integer> getExitValue() {
return process != null && !process.isAlive()
? Optional.of(process.exitValue())
: Optional.empty();
Subprocess p = this.process; // Protects against this.process getting null.
return p != null && !p.isAlive() ? Optional.of(p.exitValue()) : Optional.empty();
}

/** For testing only, to verify that maps are cleared after responses are reaped. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ public void close() {

@Override
public boolean isAlive() {
return wasDestroyed;
return !wasDestroyed;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ public void testGetResponse_slowProxy()

WorkResponse fakedResponse1 = WorkResponse.newBuilder().setRequestId(3).build();
WorkResponse fakedResponse2 = WorkResponse.newBuilder().setRequestId(42).build();
// Responses can arrive out of order
// Responses can arrive out of order, and before the workerproxies are ready to get them.
fakedResponse2.writeDelimitedTo(workerOutputStream);
fakedResponse1.writeDelimitedTo(workerOutputStream);
workerOutputStream.flush();
Expand Down

0 comments on commit 780e27d

Please sign in to comment.