Okhttp: Call.cancel shouldn't offend strict mode

[swankjesse]

We should document that it doesn't block, and get our friends at AOSP to fix strict mode to not issue a false positive warning.

[nfuller]

Do you have a stack trace? Is it one of these cases where it calls a potentially blocking POSIX call that, in practice, will never block with the way the sockets are configured?

[swankjesse]

It's related to this issue, which has come up again: https://github.com/square/okhttp/issues/869

[nfuller]

I'm not up on conscrypt. From a quick skim of the code: when it closes it looks like it sets the socket into blocking mode, then calls SSL_shutdown, which is documented to potentially block. If that's correct it looks like the call to block guard is at least correct with the current implementation.

What makes you think it's a false positive?

[swankjesse]

Hmmm . . . . What I want is to quickly & violently break a socket that isn't needed anymore. This isn't intended to be a clean shutdown that flushes queued bytes; instead it just severs the network. Perhaps for SSL we need to break the underlying socket only, which shouldn't require any blocking.

[nfuller]

The block guard code for close() on PlainSocket appears to be smart enough to check for linger and only complain if they have it set. AFAIK, the HTTP sockets shouldn't have it set, so it will be classed as non-blocking.

[Wavesonics]

I've got a stack if it helps:

android.os.NetworkOnMainThreadException
at android.os.StrictMode$AndroidBlockGuardPolicy.onNetwork(StrictMode.java:1145)
at com.android.org.conscrypt.OpenSSLSocketImpl.close(OpenSSLSocketImpl.java:1009)
at com.squareup.okhttp.Connection.closeIfOwnedBy(Connection.java:142)
at com.squareup.okhttp.OkHttpClient$1.closeIfOwnedBy(OkHttpClient.java:71)
at com.squareup.okhttp.internal.http.HttpConnection.closeIfOwnedBy(HttpConnection.java:136)
at com.squareup.okhttp.internal.http.HttpTransport.disconnect(HttpTransport.java:135)
at com.squareup.okhttp.internal.http.HttpEngine.disconnect(HttpEngine.java:507)
at com.squareup.okhttp.Call.cancel(Call.java:122)
...

[iNoles]

@Wavesonics you supposed to run any network call on background thread.

[JakeWharton]

@iNoles that is not the issue

[Wavesonics]

@iNoles the issue is that calling cancel() on a connection from the main thread triggers the OS's block guard. This is a quick call and shouldn't trigger that protection.

[swankjesse]

Should be pretty straightforward after this PR lands:
https://github.com/square/okhttp/pull/1782

We'll need to keep both the raw socket and the SSL socket. When closing asynchronously we close the raw socket; when cleaning up or closing synchronously we close both.

[swankjesse]

Also we should be precise around concurrency on access to the transport & connection. Right now it's not safe.

[swankjesse]

I'm going to punt this to 3.0, only because not involving HttpURLConnection will make this much easier.

[lawloretienne]

Are there any updates on this? I'm also getting a android.os.NetworkOnMainThreadException from Call.cancel()

09-15 15:52:20.205 6069-6069/com.etiennelawlor.loop E/AndroidRuntime: FATAL EXCEPTION: main
09-15 15:52:20.205 6069-6069/com.etiennelawlor.loop E/AndroidRuntime: Process: com.etiennelawlor.loop, PID: 6069
09-15 15:52:20.205 6069-6069/com.etiennelawlor.loop E/AndroidRuntime: android.os.NetworkOnMainThreadException
09-15 15:52:20.205 6069-6069/com.etiennelawlor.loop E/AndroidRuntime:     at android.os.StrictMode$AndroidBlockGuardPolicy.onNetwork(StrictMode.java:1273)
09-15 15:52:20.205 6069-6069/com.etiennelawlor.loop E/AndroidRuntime:     at com.android.org.conscrypt.OpenSSLSocketImpl.shutdownAndFreeSslNative(OpenSSLSocketImpl.java:1126)
09-15 15:52:20.205 6069-6069/com.etiennelawlor.loop E/AndroidRuntime:     at com.android.org.conscrypt.OpenSSLSocketImpl.close(OpenSSLSocketImpl.java:1121)
09-15 15:52:20.205 6069-6069/com.etiennelawlor.loop E/AndroidRuntime:     at com.squareup.okhttp.Connection.closeIfOwnedBy(Connection.java:148)
09-15 15:52:20.205 6069-6069/com.etiennelawlor.loop E/AndroidRuntime:     at com.squareup.okhttp.OkHttpClient$1.closeIfOwnedBy(OkHttpClient.java:75)
09-15 15:52:20.205 6069-6069/com.etiennelawlor.loop E/AndroidRuntime:     at com.squareup.okhttp.internal.http.HttpConnection.closeIfOwnedBy(HttpConnection.java:137)
09-15 15:52:20.205 6069-6069/com.etiennelawlor.loop E/AndroidRuntime:     at com.squareup.okhttp.internal.http.HttpTransport.disconnect(HttpTransport.java:135)
09-15 15:52:20.205 6069-6069/com.etiennelawlor.loop E/AndroidRuntime:     at com.squareup.okhttp.internal.http.HttpEngine.disconnect(HttpEngine.java:573)
09-15 15:52:20.205 6069-6069/com.etiennelawlor.loop E/AndroidRuntime:     at com.squareup.okhttp.Call.cancel(Call.java:122)
09-15 15:52:20.205 6069-6069/com.etiennelawlor.loop E/AndroidRuntime:     at retrofit.OkHttpCall.cancel(OkHttpCall.java:158)
09-15 15:52:20.205 6069-6069/com.etiennelawlor.loop E/AndroidRuntime:     at retrofit.ExecutorCallAdapterFactory$ExecutorCallbackCall.cancel(ExecutorCallAdapterFactory.java:63)

[lawloretienne]

I guess I have to wrap the Call.cancel() method in an AsyncTask until this gets resolved.

new CancelTask().execute(call);

private class CancelTask extends AsyncTask < Call, Void, Void > {@
    Override
    protected Void doInBackground(Call...params) {
        Call call = params[0];
        call.cancel();
        return null;
    }
}

[felipecsl]

@lawloretienne you can use the OkHttp dispatcher executor service for it:

client.getDispatcher().getExecutorService().execute(new Runnable() {
    @Override
    public void run() {
        call.cancel();
    }
});

[ubuntudroid]

Calling unsubscribeOn(Schedulers.io()) works around the issue when using rxjava with retrofit for the time being. Found the solution here.

[kzaemrio]

@ubuntudroid Thanks, it works for me

[Wavesonics]

Unfortunately all of these solutions involve doing it async. I have a use case that requires it to be deterministic so they won't work for me.

I see @swankjesse suggested punting this to 3.0, is that a wait and see about it in 3.0? Or do we expect 3.0 will inherently fix this because it's ditching HURL?

[swankjesse]

I'm working on it for OkHttp 2.7.

[Wavesonics]

@swankjesse Outstanding news! Do you have any super rough idea how much longer 2.7 will be in the oven?

[swankjesse]

4–12 weeks!