grpc
/
grpc-java
mirror of https://github.com/grpc/grpc-java.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

context: fix race between CancellableContext and Context 

The `pendingDeadline` variable is modified from the ctor of CancellableContext, but it isn't final.  The cancellation can happen before the variable is assigned.  It's generally bad practice to leak the this reference from the ctor to other threads anyways.

This code refactors the deadline calculation and scheduling so that `pendingDeadline` is modified under the lock, and the `this` reference is not exposed.

Discovered by TSAN.
This commit is contained in:
Carl Mastrangelo 2019-07-17 01:12:21 -07:00 committed by GitHub
parent b477cc2a47
commit 8a9afd618a
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
1 changed files with 37 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

46
context/src/main/java/io/grpc/Context.java
View File

@ -296,11 +296,21 @@ public class Context {
* } * }
* </pre> * </pre>
*/ */
public CancellableContext withDeadline(Deadline deadline, public CancellableContext withDeadline(Deadline newDeadline, ScheduledExecutorService scheduler) {
ScheduledExecutorService scheduler) { checkNotNull(newDeadline, "deadline");
checkNotNull(deadline, "deadline");
checkNotNull(scheduler, "scheduler"); checkNotNull(scheduler, "scheduler");
return new CancellableContext(this, deadline, scheduler); Deadline existingDeadline = getDeadline();
boolean scheduleDeadlineCancellation = true;
if (existingDeadline != null && existingDeadline.compareTo(newDeadline) <= 0) {
// The new deadline won't have an effect, so ignore it
newDeadline = existingDeadline;
scheduleDeadlineCancellation = false;
}
CancellableContext newCtx = new CancellableContext(this, newDeadline);
if (scheduleDeadlineCancellation) {
newCtx.setUpDeadlineCancellation(newDeadline, scheduler);
}
return newCtx;
} }
/** /**
@ -713,19 +723,15 @@ public class Context {
/** /**
* Create a cancellable context that has a deadline. * Create a cancellable context that has a deadline.
*/ */
private CancellableContext(Context parent, Deadline deadline, private CancellableContext(Context parent, Deadline deadline) {
ScheduledExecutorService scheduler) {
super(parent, parent.keyValueEntries); super(parent, parent.keyValueEntries);
Deadline parentDeadline = parent.getDeadline(); this.deadline = deadline;
if (parentDeadline != null && parentDeadline.compareTo(deadline) <= 0) { this.uncancellableSurrogate = new Context(this, keyValueEntries);
// The new deadline won't have an effect, so ignore it }
deadline = parentDeadline;
} else { private void setUpDeadlineCancellation(Deadline deadline, ScheduledExecutorService scheduler) {
// The new deadline has an effect
if (!deadline.isExpired()) { if (!deadline.isExpired()) {
// The parent deadline was after the new deadline so we need to install a listener final class CancelOnExpiration implements Runnable {
// on the new earlier deadline to trigger expiration for this context.
pendingDeadline = deadline.runOnExpiration(new Runnable() {
@Override @Override
public void run() { public void run() {
try { try {
@ -734,16 +740,16 @@ public class Context {
log.log(Level.SEVERE, "Cancel threw an exception, which should not happen", t); log.log(Level.SEVERE, "Cancel threw an exception, which should not happen", t);
} }
} }
}, scheduler); }
synchronized (this) {
pendingDeadline = deadline.runOnExpiration(new CancelOnExpiration(), scheduler);
}
} else { } else {
// Cancel immediately if the deadline is already expired. // Cancel immediately if the deadline is already expired.
cancel(new TimeoutException("context timed out")); cancel(new TimeoutException("context timed out"));
} }
} }
this.deadline = deadline;
uncancellableSurrogate = new Context(this, keyValueEntries);
}
@Override @Override
public Context attach() { public Context attach() {