opendev/gerrit
Code Issues Proposed changes

Fix callback group implementation to match the documentation.

Browse Source 
The contract for a CallbackGroup is that all callbacks are invoked
exactly once (either success or failure).
  + Failures are invoked immediately
  + Success wait until all callbacks are complete

In certain situations, like caches, callbacks may be invoked immediately
if the result is known. Depending on how the other callbacks completed,
this could have caused the callback to be invoked more than once; once for
success and again for the subsequently added callback success or failure.

Change-Id: If4c5d99bff6dc916826572ddb6fdff914536a01d
This commit is contained in:
Colby Ranger
2013-06-24 16:45:27 -07:00
parent a2b71c85b1
commit 332e67cd05
8 changed files with 106 additions and 78 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
gerrit-gwtui/src/main/java/com/google/gerrit/client/account/AccountApi.java
View File

@@ -76,6 +76,7 @@ public class AccountApi {
.delete(group.add(cb));
cb = CallbackGroup.emptyCallback();
}
group.done();
}
/** Retrieve the HTTP password */

2
gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/ProjectAccessScreen.java
View File

@@ -131,7 +131,7 @@ public class ProjectAccessScreen extends ProjectScreen {
}
}));
Util.PROJECT_SVC.projectAccess(getProjectKey(),
cbs.addGwtjsonrpc(new ScreenLoadCallback<ProjectAccess>(this) {
cbs.addFinal(new ScreenLoadCallback<ProjectAccess>(this) {
@Override
public void preDisplay(ProjectAccess access) {
displayReadOnly(access);

2
gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeScreen.java
View File

@@ -288,7 +288,7 @@ public class ChangeScreen extends Screen
// Handled by last callback's onFailure.
}
}));
ChangeApi.detail(event.getValue().getChange().getId().get(), cbs.add(
ChangeApi.detail(event.getValue().getChange().getId().get(), cbs.addFinal(
new GerritCallback<com.google.gerrit.client.changes.ChangeInfo>() {
@Override
public void onSuccess(

2
gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PublishCommentScreen.java
View File

@@ -163,7 +163,7 @@ public class PublishCommentScreen extends AccountScreen implements
// Handled by ScreenLoadCallback.onFailure().
}
}));
Util.DETAIL_SVC.patchSetPublishDetail(patchSetId, cbs.addGwtjsonrpc(
Util.DETAIL_SVC.patchSetPublishDetail(patchSetId, cbs.addFinal(
new ScreenLoadCallback<PatchSetPublishDetail>(this) {
@Override
protected void preDisplay(final PatchSetPublishDetail result) {

2
gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CodeMirrorDemo.java
View File

@@ -79,7 +79,7 @@ public class CodeMirrorDemo extends Screen {
.wholeFile()
.intraline()
.ignoreWhitespace(DiffApi.IgnoreWhitespace.NONE)
.get(group.add(new GerritCallback<DiffInfo>() {
.get(group.addFinal(new GerritCallback<DiffInfo>() {
@Override
public void onSuccess(final DiffInfo diff) {
new ModeInjector()

2
gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchScreen.java
View File

@@ -386,7 +386,7 @@ public abstract class PatchScreen extends Screen implements
}
}));
PatchUtil.DETAIL_SVC.patchScript(patchKey, idSideA, idSideB,
settingsPanel.getValue(), cb.addGwtjsonrpc(
settingsPanel.getValue(), cb.addFinal(
new ScreenLoadCallback<PatchScript>(this) {
@Override
protected void preDisplay(final PatchScript result) {

166
gerrit-gwtui/src/main/java/com/google/gerrit/client/rpc/CallbackGroup.java
View File

@@ -17,15 +17,18 @@ package com.google.gerrit.client.rpc;
import com.google.gwt.user.client.rpc.AsyncCallback;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
/**
* Class for grouping together callbacks and calling them in order.
* <p>
* Callbacks are added to the group with {@link #add(AsyncCallback)}, which
* returns a wrapped callback suitable for passing to an asynchronous RPC call.
* The last callback must be added using {@link #addFinal(AsyncCallback)} or
* {@link #done()} must be invoked.
*
* The enclosing group buffers returned results and ensures that
* {@code onSuccess} is called exactly once for each callback in the group, in
* the same order that callbacks were added. This allows callers to, for
@@ -39,12 +42,15 @@ import java.util.Map;
* processing it.
*/
public class CallbackGroup {
private final List<Object> callbacks;
private final Map<Object, Object> results;
private boolean failed;
private final List<CallbackImpl<?>> callbacks;
private final Set<CallbackImpl<?>> remaining;
private boolean finalAdded;
public static <T> AsyncCallback<T> emptyCallback() {
return new AsyncCallback<T>() {
private boolean failed;
private Throwable failedThrowable;
public static <T> Callback<T> emptyCallback() {
return new Callback<T>() {
@Override
public void onSuccess(T result) {
}
@@ -56,77 +62,99 @@ public class CallbackGroup {
}
public CallbackGroup() {
callbacks = new ArrayList<Object>();
results = new HashMap<Object, Object>();
callbacks = new ArrayList<CallbackImpl<?>>();
remaining = new HashSet<CallbackImpl<?>>();
}
public <T> AsyncCallback<T> add(final AsyncCallback<T> cb) {
callbacks.add(cb);
return new AsyncCallback<T>() {
@Override
public void onSuccess(T result) {
results.put(cb, result);
CallbackGroup.this.onSuccess();
public <T> Callback<T> add(final AsyncCallback<T> cb) {
checkFinalAdded();
return handleAdd(cb);
}
public <T> Callback<T> addFinal(final AsyncCallback<T> cb) {
checkFinalAdded();
finalAdded = true;
return handleAdd(cb);
}
public void done() {
finalAdded = true;
applyAllSuccess();
}
private void applyAllSuccess() {
if (!failed && finalAdded && remaining.isEmpty()) {
for (CallbackImpl<?> cb : callbacks) {
cb.applySuccess();
}
callbacks.clear();
}
}
private <T> Callback<T> handleAdd(AsyncCallback<T> cb) {
if (failed) {
cb.onFailure(failedThrowable);
return emptyCallback();
}
CallbackImpl<T> wrapper = new CallbackImpl<T>(cb);
callbacks.add(wrapper);
remaining.add(wrapper);
return wrapper;
}
private void checkFinalAdded() {
if (finalAdded) {
throw new IllegalStateException("final callback already added");
}
}
public interface Callback<T>
extends AsyncCallback<T>, com.google.gwtjsonrpc.common.AsyncCallback<T> {
}
private class CallbackImpl<T> implements Callback<T> {
AsyncCallback<T> delegate;
T result;
CallbackImpl(AsyncCallback<T> delegate) {
this.delegate = delegate;
}
@Override
public void onFailure(Throwable caught) {
CallbackGroup.this.onFailure(caught);
}
};
}
public <T> com.google.gwtjsonrpc.common.AsyncCallback<T> addGwtjsonrpc(
final com.google.gwtjsonrpc.common.AsyncCallback<T> cb) {
callbacks.add(cb);
return new com.google.gwtjsonrpc.common.AsyncCallback<T>() {
@Override
public void onSuccess(T result) {
results.put(cb, result);
CallbackGroup.this.onSuccess();
}
@Override
public void onFailure(Throwable caught) {
CallbackGroup.this.onFailure(caught);
}
};
}
private void onSuccess() {
if (results.size() < callbacks.size()) {
return;
}
for (Object o : callbacks) {
Object result = results.get(o);
if (o instanceof AsyncCallback) {
@SuppressWarnings("unchecked")
AsyncCallback<Object> cb = (AsyncCallback<Object>) o;
cb.onSuccess(result);
} else {
@SuppressWarnings("unchecked")
com.google.gwtjsonrpc.common.AsyncCallback<Object> cb =
(com.google.gwtjsonrpc.common.AsyncCallback<Object>) o;
cb.onSuccess(result);
}
}
}
private void onFailure(Throwable caught) {
public void onSuccess(T value) {
if (failed) {
return;
}
this.result = value;
remaining.remove(this);
CallbackGroup.this.applyAllSuccess();
}
@Override
public void onFailure(Throwable caught) {
if (failed) {
return;
}
failed = true;
for (Object o : callbacks) {
if (o instanceof AsyncCallback) {
@SuppressWarnings("unchecked")
AsyncCallback<Object> cb = (AsyncCallback<Object>) o;
cb.onFailure(caught);
} else {
@SuppressWarnings("unchecked")
com.google.gwtjsonrpc.common.AsyncCallback<Object> cb =
(com.google.gwtjsonrpc.common.AsyncCallback<Object>) o;
cb.onFailure(caught);
failedThrowable = caught;
for (CallbackImpl<?> cb : callbacks) {
cb.delegate.onFailure(failedThrowable);
cb.delegate = null;
cb.result = null;
}
callbacks.clear();
remaining.clear();
}
void applySuccess() {
AsyncCallback<T> cb = delegate;
if (cb != null) {
delegate = null;
cb.onSuccess(result);
result = null;
}
}
}

13
gerrit-gwtui/src/main/java/net/codemirror/lib/Loader.java vendored
View File

@@ -41,13 +41,12 @@ class Loader {
} else {
CallbackGroup group = new CallbackGroup();
injectCss(Lib.I.css());
injectScript(
Lib.I.js().getSafeUri(), group.add(new AsyncCallback<Void>() {
public void onFailure(Throwable caught) {}
public void onSuccess(Void result) {}
}));
injectScript(Addons.I.mark_selection().getSafeUri(), group.add(cb));
injectScript(Addons.I.foldcode().getSafeUri(), group.add(cb));
injectScript(Lib.I.js().getSafeUri(),
group.add(CallbackGroup.<Void>emptyCallback()));
injectScript(Addons.I.mark_selection().getSafeUri(),
group.add(CallbackGroup.<Void>emptyCallback()));
injectScript(Addons.I.foldcode().getSafeUri(),
group.addFinal(cb));
}
}