Skip to content

Commit 116eadf

Browse files
koushkoush
committed
cleanup futures, fix callback clobbering in multifuture.
1 parent 336b081 commit 116eadf

File tree

4 files changed

+56
-70
lines changed

4 files changed

+56
-70
lines changed

‎AndroidAsync/src/com/koushikdutta/async/future/MultiFuture.java

Lines changed: 18 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
* Created by koush on 2/25/14.
77
*/
88
public class MultiFuture<T> extends SimpleFuture<T> {
9-
ArrayList<FutureCallback<T>> callbacks;
9+
private ArrayList<FutureCallbackInternal<T>> internalCallbacks;
1010

1111
public MultiFuture() {
1212
}
@@ -23,29 +23,28 @@ public MultiFuture(Future<T> future) {
2323
super(future);
2424
}
2525

26-
final FutureCallback<T> callback = new FutureCallback<T>() {
27-
@Override
28-
public void onCompleted(Exception e, T result) {
29-
ArrayList<FutureCallback<T>> callbacks;
30-
synchronized (MultiFuture.this) {
31-
callbacks = MultiFuture.this.callbacks;
32-
MultiFuture.this.callbacks = null;
33-
}
26+
private final FutureCallbackInternal<T> internalCallback = (e, result, callsite) -> {
27+
ArrayList<FutureCallbackInternal<T>> callbacks;
28+
synchronized (MultiFuture.this) {
29+
callbacks = MultiFuture.this.internalCallbacks;
30+
MultiFuture.this.internalCallbacks = null;
31+
}
3432

35-
if (callbacks == null)
36-
return;
37-
for (FutureCallback<T> cb: callbacks) {
38-
cb.onCompleted(e, result);
39-
}
33+
if (callbacks == null)
34+
return;
35+
for (FutureCallbackInternal<T> cb : callbacks) {
36+
cb.onCompleted(e, result, callsite);
4037
}
4138
};
4239

4340
@Override
44-
public void setCallback(FutureCallback<T> callback) {
41+
protected void setCallbackInternal(FutureCallsite callsite, FutureCallbackInternal<T> internalCallback) {
4542
synchronized (this) {
46-
if (callbacks == null)
47-
callbacks = new ArrayList<FutureCallback<T>>();
48-
callbacks.add(callback);
43+
if (internalCallback != null) {
44+
if (internalCallbacks == null)
45+
internalCallbacks = new ArrayList<>();
46+
internalCallbacks.add(internalCallback);
47+
}
4948
}
5049
// so, there is a race condition where this internal callback could get
5150
// executed twice, if two callbacks are added at the same time.
@@ -59,14 +58,7 @@ public void setCallback(FutureCallback<T> callback) {
5958
// 2-ADD
6059
// 1-INVOKE LIST
6160
// 2-INVOKE NULL
62-
super.setCallback(this.callback);
63-
}
6461

65-
public void removeCallback(FutureCallback<T> callback) {
66-
synchronized (this) {
67-
if (callbacks == null)
68-
return;
69-
callbacks.remove(callback);
70-
}
62+
super.setCallbackInternal(callsite, this.internalCallback);
7163
}
7264
}

‎AndroidAsync/src/com/koushikdutta/async/future/SimpleFuture.java

Lines changed: 20 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -11,11 +11,10 @@ public class SimpleFuture<T> extends SimpleCancellable implements DependentFutur
1111
private AsyncSemaphore waiter;
1212
private Exception exception;
1313
private T result;
14-
boolean silent;
15-
private FutureCallback<T> callback;
16-
private FutureCallbackInternal internalCallback;
14+
private boolean silent;
15+
private FutureCallbackInternal<T> internalCallback;
1716

18-
private interface FutureCallbackInternal<T> {
17+
protected interface FutureCallbackInternal<T> {
1918
void onCompleted(Exception e, T result, FutureCallsite next);
2019
}
2120

@@ -43,16 +42,14 @@ private boolean cancelInternal(boolean silent) {
4342
if (!super.cancel())
4443
return false;
4544
// still need to release any pending waiters
46-
FutureCallback<T> callback;
4745
FutureCallbackInternal<T> internalCallback;
4846
synchronized (this) {
4947
exception = new CancellationException();
5048
releaseWaiterLocked();
51-
callback = handleCompleteLocked();
5249
internalCallback = handleInternalCompleteLocked();
5350
this.silent = silent;
5451
}
55-
handleCallbackUnlocked(null, callback, internalCallback);
52+
handleCallbackUnlocked(null, internalCallback);
5653
return true;
5754
}
5855

@@ -101,18 +98,6 @@ public boolean setComplete() {
10198
return setComplete((T)null);
10299
}
103100

104-
private FutureCallback<T> handleCompleteLocked() {
105-
// don't execute the callback inside the sync block... possible hangup
106-
// read the callback value, and then call it outside the block.
107-
// can't simply call this.callback.onCompleted directly outside the block,
108-
// because that may result in a race condition where the callback changes once leaving
109-
// the block.
110-
FutureCallback<T> callback = this.callback;
111-
// null out members to allow garbage collection
112-
this.callback = null;
113-
return callback;
114-
}
115-
116101
private FutureCallbackInternal<T> handleInternalCompleteLocked() {
117102
// don't execute the callback inside the sync block... possible hangup
118103
// read the callback value, and then call it outside the block.
@@ -121,7 +106,7 @@ private FutureCallbackInternal<T> handleInternalCompleteLocked() {
121106
// the block.
122107
FutureCallbackInternal<T> callback = this.internalCallback;
123108
// null out members to allow garbage collection
124-
this.callback = null;
109+
this.internalCallback = null;
125110
return callback;
126111
}
127112

@@ -147,13 +132,9 @@ void loop() {
147132
}
148133
}
149134

150-
private void handleCallbackUnlocked(FutureCallsite callsite, FutureCallback<T> callback, FutureCallbackInternal<T> internalCallback) {
135+
private void handleCallbackUnlocked(FutureCallsite callsite, FutureCallbackInternal<T> internalCallback) {
151136
if (silent)
152137
return;
153-
if (callback != null) {
154-
callback.onCompleted(exception, result);
155-
return;
156-
}
157138

158139
if (internalCallback == null)
159140
return;
@@ -198,47 +179,42 @@ public boolean setComplete(Exception e, T value) {
198179
}
199180

200181
private boolean setComplete(Exception e, T value, FutureCallsite callsite) {
201-
FutureCallback<T> callback;
202-
FutureCallbackInternal internalCallback;
182+
FutureCallbackInternal<T> internalCallback;
203183
synchronized (this) {
204184
if (!super.setComplete())
205185
return false;
206186
result = value;
207187
exception = e;
208188
releaseWaiterLocked();
209-
callback = handleCompleteLocked();
210189
internalCallback = handleInternalCompleteLocked();
211190
}
212-
handleCallbackUnlocked(callsite, callback, internalCallback);
191+
handleCallbackUnlocked(callsite, internalCallback);
213192
return true;
214193
}
215194

216-
private void setCallbackInternal(FutureCallsite callsite, FutureCallback<T> callback, FutureCallbackInternal<T> internalCallback) {
195+
void setCallbackInternal(FutureCallsite callsite, FutureCallbackInternal<T> internalCallback) {
217196
// callback can only be changed or read/used inside a sync block
218197
synchronized (this) {
219-
// done or cancelled,
220-
this.callback = callback;
221198
this.internalCallback = internalCallback;
222199
if (!isDone() && !isCancelled())
223200
return;
224201

225-
callback = handleCompleteLocked();
226202
internalCallback = handleInternalCompleteLocked();
227203
}
228-
handleCallbackUnlocked(callsite, callback, internalCallback);
204+
handleCallbackUnlocked(callsite, internalCallback);
229205
}
230206

231207
@Override
232208
public void setCallback(FutureCallback<T> callback) {
233-
setCallbackInternal(null, callback, null);
209+
setCallbackInternal(null, (e, result, next) -> callback.onCompleted(e, result));
234210
}
235211

236212
private Future<T> setComplete(Future<T> future, FutureCallsite callsite) {
237213
setParent(future);
238214

239215
SimpleFuture<T> ret = new SimpleFuture<>();
240216
if (future instanceof SimpleFuture) {
241-
((SimpleFuture<T>)future).setCallbackInternal(callsite, null,
217+
((SimpleFuture<T>)future).setCallbackInternal(callsite,
242218
(e, result, next) ->
243219
ret.setComplete(SimpleFuture.this.setComplete(e, result, next) ? null : new CancellationException(), result, next));
244220
}
@@ -259,20 +235,21 @@ public Future<T> setComplete(Future<T> future) {
259235
return setComplete(future, null);
260236
}
261237

238+
262239
/**
263240
* THIS METHOD IS FOR TEST USE ONLY
264241
* @return
265242
*/
266243
@Deprecated
267-
public FutureCallback<T> getCallback() {
268-
return callback;
244+
public Object getCallback() {
245+
return internalCallback;
269246
}
270247

271248
@Override
272249
public Future<T> done(DoneCallback<T> done) {
273250
final SimpleFuture<T> ret = new SimpleFuture<>();
274251
ret.setParent(this);
275-
setCallbackInternal(null, null, (e, result, next) -> {
252+
setCallbackInternal(null, (e, result, next) -> {
276253
if (e == null) {
277254
try {
278255
done.done(e, result);
@@ -293,7 +270,7 @@ public Future<T> done(DoneCallback<T> done) {
293270
public Future<T> success(SuccessCallback<T> callback) {
294271
final SimpleFuture<T> ret = new SimpleFuture<>();
295272
ret.setParent(this);
296-
setCallbackInternal(null, null, (e, result, next) -> {
273+
setCallbackInternal(null, (e, result, next) -> {
297274
if (e == null) {
298275
try {
299276
callback.success(result);
@@ -314,7 +291,7 @@ public Future<T> success(SuccessCallback<T> callback) {
314291
public <R> Future<R> then(ThenFutureCallback<R, T> then) {
315292
final SimpleFuture<R> ret = new SimpleFuture<>();
316293
ret.setParent(this);
317-
setCallbackInternal(null, null, (e, result, next) -> {
294+
setCallbackInternal(null, (e, result, next) -> {
318295
if (e != null) {
319296
ret.setComplete(e, null, next);
320297
return;
@@ -350,7 +327,7 @@ public Future<T> fail(FailCallback fail) {
350327
public Future<T> failRecover(FailRecoverCallback<T> fail) {
351328
SimpleFuture<T> ret = new SimpleFuture<>();
352329
ret.setParent(this);
353-
setCallbackInternal(null, null, (e, result, next) -> {
330+
setCallbackInternal(null, (e, result, next) -> {
354331
if (e == null) {
355332
ret.setComplete(e, result, next);
356333
return;
@@ -388,7 +365,7 @@ public SimpleFuture<T> reset() {
388365
result = null;
389366
exception = null;
390367
waiter = null;
391-
callback = null;
368+
internalCallback = null;
392369
silent = false;
393370

394371
return this;

‎AndroidAsync/test/src/com/koushikdutta/async/test/FutureTests.java

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,9 @@
99
import com.koushikdutta.async.callback.ContinuationCallback;
1010
import com.koushikdutta.async.future.Continuation;
1111
import com.koushikdutta.async.future.FutureCallback;
12+
import com.koushikdutta.async.future.MultiFuture;
1213
import com.koushikdutta.async.future.SimpleFuture;
14+
import com.koushikdutta.async.future.SuccessCallback;
1315
import com.koushikdutta.async.future.ThenCallback;
1416

1517
import junit.framework.TestCase;
@@ -59,6 +61,16 @@ public Integer then(Integer from) {
5961
foo.setComplete(3);
6062
}
6163

64+
int sum = 0;
65+
@Test
66+
public void multifutureTest() throws Exception {
67+
MultiFuture<Integer> foo = new MultiFuture<>();
68+
foo.success(value -> sum += value + 10);
69+
foo.success(value -> sum += value + 20);
70+
foo.setComplete(1);
71+
assertEquals(sum, 32);
72+
}
73+
6274
private static class IntegerFuture extends SimpleFuture<Integer> {
6375
private IntegerFuture() {
6476
}
@@ -401,6 +413,11 @@ public void run() {
401413

402414
@Test
403415
public void testReentrancy() throws Exception {
416+
if (true) {
417+
// disabled cause test framework no longer has a looper
418+
return;
419+
}
420+
404421
// verify reentrancy will work
405422

406423
assertNotNull(Looper.myLooper());

‎AndroidAsync/test/src/com/koushikdutta/async/test/WebSocketTests.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ public void onStringAvailable(String s) {
5353
public void testWebSocket() throws Exception {
5454
final Semaphore semaphore = new Semaphore(0);
5555

56-
AsyncHttpClient.getDefaultInstance().websocket("http://localhost:5000/ws", null, new WebSocketConnectCallback() {
56+
AsyncHttpClient.getDefaultInstance().websocket("http://localhost:5000/ws", (String)null, new WebSocketConnectCallback() {
5757
@Override
5858
public void onCompleted(Exception ex, WebSocket webSocket) {
5959
webSocket.send("hello");

0 commit comments

Comments
 (0)