google / volley

https://google.github.io/volley
Apache License 2.0
3.38k stars 754 forks source link

RequestFuture#get blocks forever if an in-flight request is cancelled #85

Open jpd236 opened 7 years ago

jpd236 commented 7 years ago

RequestFuture#get waits for either onResponse or onErrorResponse to be called before attempting to return a result. However, if a request is cancelled, by design, neither callback will be executed. This means get() will block forever (or until the user-specified timeout, if one is given).

Proposal to fix:

jpd236 commented 7 years ago

Unfortunately I think the work to add a proper CancelListener and have it adhere to a reasonable contract is a bit larger than I'd like to include in 1.0.1.

103 is my initial attempt at this, which had some comments that would need to be addressed. In particular, if we're going to expose this API externally, we should provide some reasonable guarantees about the thread we invoke the listener on, ideally in a consistent way with what we do for response / error response callbacks. On the other hand, if we implement obfuscation as in #102, we could potentially keep this API private to Volley for now and only use it to prevent RequestFuture from blocking forever

jpd236 commented 6 years ago

I took one more look to see if we could do this in any way that wouldn't require any API changes.

Short of a proper CancelListener interface, we need some way of calling notifyAll() on the RequestFuture after the request has been canceled. This would then have to go through Request#cancel. However, checking whether mErrorListener is an instance of RequestFuture there would introduce a dependency from the base volley package to the toolbox package, which historically we have not done. (I question whether this matters, but wouldn't change this for a point fix). And calling notifyAll() unconditionally on any ErrorListener could lead to unexpected behavior for listeners other than ErrorListener who are relying on wait/notify for their own internal semantics. (In a well-behaved app, it shouldn't, because spurious wakeups are always a possibility, but again, we don't want to be introducing risk like that in a point-fix release). We can't create an interface for RequestFuture to implement because this would be a new public API (so that RequestFuture could depend on it from a different package).

We could fix RequestFuture#cancel, but this doesn't really do much to address the problem IMO which is going through RequestQueue's cancel interface while something else is waiting on the future.

So - leaving this on the 1.2.0 milestone.