RequestFuture#get blocks forever if an in-flight request is cancelled #85
Comments
- Provide stronger thread safety guarantees by locking access to any variable which can be mutated during the processing of a request (i.e. by the CacheDispatcher/NetworkDispatcher threads) and dispatching response callbacks while holding the same lock that is held when setting mCanceled. Unfortunately there's not much we can do about alternative ResponseDelivery mechanisms, which are hopefully rare. - Add a cancel listener interface and use it to prevent RequestFuture from blocking for the complete timeout (or indefinitely) if a request is canceled. - Since listeners are often Android component classes like Activitys, clear them upon cancellation. Unfortunately, this must be done in each Request subclass to work, assuming the subclass follows the standard pattern of holding the response listener as a member. But this enables concerned apps to resolve this leak. Fixes google#15 Fixes google#85
- Provide stronger thread safety guarantees by locking access to any variable which can be mutated during the processing of a request (i.e. by the CacheDispatcher/NetworkDispatcher threads) and dispatching response callbacks while holding the same lock that is held when setting mCanceled. Unfortunately there's not much we can do about alternative ResponseDelivery mechanisms, which are hopefully rare. - Add a cancel listener interface and use it to prevent RequestFuture from blocking for the complete timeout (or indefinitely) if a request is canceled. - Since listeners are often Android component classes like Activitys, clear them upon cancellation. Unfortunately, this must be done in each Request subclass to work, assuming the subclass follows the standard pattern of holding the response listener as a member. But this enables concerned apps to resolve this leak. Fixes google#15 Fixes google#85
|
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 |
|
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. |
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:
The text was updated successfully, but these errors were encountered: