okhttp 2.0.0 OkHttpClient.cancel(tag) doesn't work

[HeGanjie]

The Recipes Canceling a Call worked, but after I used OkHttpClient.cancel(tag) instead, it didn't work.

My code:

import java.io.IOException;
import java.util.concurrent.*;
import com.squareup.okhttp.*;

public class Test {
    private final ScheduledExecutorService executor = Executors.newScheduledThreadPool(1);
    private final OkHttpClient client = new OkHttpClient();

    public void run() throws Exception {
        Request request = new Request.Builder()
        .url("http://httpbin.org/delay/2") // This URL is served with a 2 second delay.
        .tag("asdf")
        .build();

        final long startNanos = System.nanoTime();
        final Call call = client.newCall(request);

        // Schedule a job to cancel the call in 1 second.
        executor.schedule(new Runnable() {
            @Override public void run() {
                System.out.printf("%.2f Canceling call.%n", (System.nanoTime() - startNanos) / 1e9f);
                client.cancel("asdf");
                System.out.printf("%.2f Canceled call.%n", (System.nanoTime() - startNanos) / 1e9f);
            }
        }, 1, TimeUnit.SECONDS);

        try {
            System.out.printf("%.2f Executing call.%n", (System.nanoTime() - startNanos) / 1e9f);
            Response response = call.execute();
            System.out.printf("%.2f Call was expected to fail, but completed: %s%n",
                    (System.nanoTime() - startNanos) / 1e9f, response);
        } catch (IOException e) {
            System.out.printf("%.2f Call failed as expected: %s%n",
                    (System.nanoTime() - startNanos) / 1e9f, e);
        }
    }

    public static void main(String[] args) throws Exception {
        new Test().run();
    }
}

Result:

0.25 Executing call.
1.25 Canceling call.
1.25 Canceled call.
3.46 Call was expected to fail, but completed: Response{protocol=http/1.1, code=200, message=OK, url=http://httpbin.org/delay/2}

[swankjesse]

Yikes. Will fix.

[codefromthecrypt]

seems like our test of this scenario must be missing something https://github.com/square/okhttp/blob/master/okhttp-tests/src/test/java/com/squareup/okhttp/CallTest.java#L1153

[codefromthecrypt]

I'll take this

[codefromthecrypt]

looks like Dispatcher.readyCalls and runningCalls have no contents when this test's call to cancel hits. In other words, cancel is a no-op.

[codefromthecrypt]

Ok, so we have tests that cover the following

• asynchronous calls canceled with either OkHttpClient.cancel or call.cancel
• synchronous calls canceled with call.cancel

This test makes a synchronous call, then tries to cancel with OkHttpClient.cancel

[codefromthecrypt]

So we can address this in one of 2 ways.

1. change documentation for OkHttpClient.cancel to note that it only applies to enqueued requests
2. do some re-plumbing in Dispatcher, or otherwise decouple cancelByTag from AsyncCall

@swankjesse wdyt?

[codefromthecrypt]

The other tricky thing about this is that we are attempting to cancel a call that we have no idea will be ever executed.

1. client.newCall allocates a call
2. client.cancel says cancel things with this tag
3. call.execute needs to know it should be canceled.

This looks a lot like scheduling to me: schedule cancelation of this tag
What if call.execute is never made? What if call.enqueue is used instead? What if the tag on client.cancel is never used, or used more than once? The tracking needed to properly address scheduled cancelation might make memory leaks if we don't take particular care.

I'm actually ok giving this a go, but I'd like to make sure we are ok with the implications of this before I do. Or maybe someone knows a better solution.

[codefromthecrypt]

Actually, taking a close look, this does seem to only ask to cancel in-flight requests. For example, the test order is: call.execute before client.cancel.

We can do that..

[codefromthecrypt]

ok #1065 passes for this case. mildly updated cancel(tag) doc to

Cancels all scheduled or in-flight calls tagged with {@code tag}