google / volley

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

Future isn't interrupted when RequestFuture is cancelled #143

Closed blinkmacalahan closed 6 years ago

blinkmacalahan commented 6 years ago

I've been digging relatively deep into the code for about a day and it appears that cancelling the RequestFuture only cancels the request and not the Future itself. I would expect when cancelling the RequestFuture, the associated Future would throw a InterruptedException. That way my blocking code would stop blocking and move on. I've tried following what the documentation says and have set my request on the future, but an InterruptedException is never thrown. Instead, I have to wait for the TimeoutException occur which isn't ideal.

 RequestFuture<JSONObject> future = RequestFuture.newFuture();
 MyRequest request = new MyRequest(URL, future, future);

 // If you want to be able to cancel the request:
 future.setRequest(requestQueue.add(request));

try {
   JSONObject response = future.get(50,
                    TimeUnit.SECONDS);
   // do something with response
 } catch (InterruptedException e) {
   // handle the error
 } catch (ExecutionException e) {
   // handle the error
 }

As I mentioned I've been digging into the code a while but my apologies if I'm missing something. Thanks!

jpd236 commented 6 years ago

Sorry that you ended up spending a day on this... unfortunately it's a known issue (#85). Fixing it is unfortunately non-trivial as noted there.

blinkmacalahan commented 6 years ago

I did see that issue (#85) but felt it was different than what I'm suggesting. I feel like adding a Cancel listener is completely different than expecting the Future itself to get canceled. However, possibly the work to do both is related so I'll leave it to your discretion.

As a future FYI to others that might stumble across this thread, an extremely hacky way to work-around this issue is if the request is already in flight, when you go to cancel your future, you can also return null to the Future's onResponse. Make sure your code handling the response looks for null and handles it as a failure.

jpd236 commented 6 years ago

The issue is that RequestFuture (which is just a helper class in the .toolbox package) needs to know when the request was canceled in order to wake up the sleeping monitor in get() - that's what the cancel listener would be used for.