WARNING: A connection to https://... was leaked. Did you forget to close a response body?

[bxqgit]

Version:
com.squareup.okhttp3:okhttp:3.0.1

Log message:

<timestamp> okhttp3.ConnectionPool pruneAndGetAllocationCount
WARNING: A connection to https://... was leaked. Did you forget to close a response body?

Code:

private String get(final String url) {
    String res = null;
    try {
        final Request req = new Request.Builder().url(url).get().build();
        final Response resp = ok.newCall(req).execute();
        final int code = resp.code();
        if (code == 200) {
            final ResponseBody body = resp.body();
            res = body.string();
            body.close(); // even this doesn't work!
        }
    }
    catch (final Throwable th) {
        System.out.println(th.getMessage());
    }
    return res;
}

[JakeWharton]

That code does not close the body when the code is not 200. Use a try/finally with body.close() in the finally block.

[bxqgit]

1. If the code is not 200 -> there's no call to resp.body() -> there's no body -> there's nothing to close. Is it so?
2. I thought I don't have to call body.close() at all! I just put it there for an experiment. Do I have to call body.close() everytime?

[swankjesse]

Everytime.

[bxqgit]

Ok, it's called eveytime, as you see in my code. But I still get that warning.

[bxqgit]

I get that warning from time to time. I make like 100 calls and get 1 warning. 99 calls are ok.

[bxqgit]

Code is the same in all 100 calls.

[JakeWharton]

Your code calls it only in the success case, which is not every time.

If the code is not 200 -> there's no call to resp.body() -> there's no body -> there's nothing to close.

This is false.

[bxqgit]

I commented each valuable line.

private String get(final String url) {
    String res = null;
    try {
        final Request req = new Request.Builder().url(url).get().build();
        final Response resp = ok.newCall(req).execute();
        final int code = resp.code(); // can be any value
        if (code == 200) {
            final ResponseBody body = resp.body(); // body exists, I have to close it
            res = body.string(); // 
            body.close(); // I close it explicitly
        }
        // if code was not 200 -> there's no body (opened to be closed) -> there's nothing to close
        // What am I supposed to close?
    }
    catch (final Throwable th) {
        System.out.println(th.getMessage());
    }
    return res;
}

[JakeWharton]

You have to close the body. Just because you are not calling the method doesn't mean the body is not there.

ResponseBody body = resp.body();
try {
  ...
} finally {
 body.close();
}

Ignoring this fact, the code you showed doesn't cover the case when body.string() throws either.

[bxqgit]

What about this case:

private String get(final String url) {
    String res = null;
    try {
        final Request req = new Request.Builder().url(url).get().build();
        final Response resp = ok.newCall(req).execute();
        final int code = resp.code(); // can be any value
        System.out.println(code); // I don't want to get body from resp, just print the code
        // There's no body, there's nothing opened, there's nothing to close
        // Do I have to call resp.body().close() anyway in finally block or something like that?
    }
    catch (final Throwable th) {
        System.out.println(th.getMessage());
    }
    return res;
}

[JakeWharton]

there's nothing opened

The connection is open. That's where the code came from. You always have to close the body. There's nothing ambiguous about "always".

[bxqgit]

#2158 (comment)
"Most call body().string() which closes it for you."
How is that?

[JakeWharton]

Calling string() closes the body, yes.

[bxqgit]

So, just to read the code of the get response, I have to write something like:

private void printCode(final String url) {
    Response resp = null;
    try {
        final Request req = new Request.Builder().url(url).get().build();
        resp = ok.newCall(req).execute();
        final int code = resp.code();
        System.out.println(code);
    }
    catch (final Throwable th) {
        System.out.println(th.getMessage());
    }
    finally {
        if (resp != null) {
            try {
                resp.body().close();
            }
            catch (final Throwable th) {
                System.out.println(th.getMessage());
            }
        }
    }
}

[JakeWharton]

Nope

Request req = new Request.Builder().url(url).build();
Response res = ok.newCall(req).execute();
System.out.println(resp.code());
res.body().close();

[JakeWharton]

The question was answered earlier, so I'm closing the issue.

[bxqgit]

So this is the right one:

private String get(final String url) {
    String res = null;
    try {
        final Request req = new Request.Builder().url(url).get().build();
        final Response resp = ok.newCall(req).execute();
        final int code = resp.code();
        if (code == 200) {
            try (ResponseBody body = resp.body()) {
                res = body.string();
            }
        }
    }
    catch (final Throwable th) {
        System.out.println(th.getMessage());
    }
    return res;
}

[JakeWharton]

That code doesn't close the body in non-200 cases.

[bxqgit]

That's a bit weird thing to remember, to close something you didn't explicitly open :)

[JakeWharton]

You opened it by making the request. Calling body just accesses something that already exists.

[bxqgit]

That's the right (ugly) one:

private String get(final String url) {
    String res = null;
    try {
        final Request req = new Request.Builder().url(url).get().build();
        final Response resp = ok.newCall(req).execute();
        final int code = resp.code();
        try (final ResponseBody body = resp.body()) {
            if (code == 200) {
                res = body.string();
            }
            else {
                log.warn("code: " + code);
            }
        }
    }
    catch (final Throwable th) {
        log.warn(th.getMessage());
    }
    return res;
}

[bxqgit]

So if I call body.string() and it throws exception, body remains opened and has to be closed, right?

[bxqgit]

I mean body.string() closes body implicitly only in positive case with no exceptions, and remains it opened if the exception is thrown, right?

[iNoles]

I think body.string() automatically closed.

There is like

if (!response.isSuccessful()) throw new IOException("Unexpected code " + response);

so you don't need 200 checks.

[bxqgit]

@iNoles If that was the case, the first version of the code wouldn't cause warnings, I guess.

[bxqgit]

body.string() which closes body anyway, if there's or there's no exception - that would be a nice cute API :)

[JakeWharton]

The first version doesn't close in the non-200 case.

string() always closes. You can look at the source.

On Wed, Feb 3, 2016, 6:53 PM bxqgit [email protected] wrote:

body.string() which closes body anyway, if there's or there's no exception

• that would be a nice cute API :)

—
Reply to this email directly or view it on GitHub
#2311 (comment).

[iNoles]

@bxqgit

private String get(final String url) {
    String res = null;
    try {
        final Request req = new Request.Builder().url(url).get().build();
        final Response response = ok.newCall(req).execute();
        if (!response.isSuccessful()) {
            log.warn(new IOException("Unexpected code " + response));
        } else {
            res = response.body().string();
        }
    catch (Throwable th) {
        log.warn(th.getMessage());
    }
    return res;
}

[bxqgit]

@JakeWharton ok, then this should work:

private String get(final String url) {
    String res = null;
    try {
        final Request req = new Request.Builder().url(url).get().build();
        final Response resp = ok.newCall(req).execute();
        res = resp.body().string();
        final int code = resp.code();
        if (code != 200) {
            log.warn("code: " + code);
        }
    }
    catch (final Throwable th) {
        log.warn(th.getMessage());
    }
    return res;
}

[mockturtl]

Just saw this in my logs from Firebase Crashlytics init. Hopefully relevant / authoritative:

It looks like it was decided this was a platform issue, rather than anything at the app level. As a result, the logged warning was removed from the platform in https://android-review.googlesource.com/c/platform/external/okhttp/+/460418.

[lbx2015]

I am so glad can the comments from author. In my case I am useing Response rather than ResponseBody, so how can I close .
package com.heitouyang.promise.server.callback;

import android.content.Intent;

import androidx.annotation.NonNull;
import androidx.localbroadcastmanager.content.LocalBroadcastManager;

import com.heitouyang.promise.activity.LoginActivity;
import com.heitouyang.promise.app.ZActivityManager;
import com.heitouyang.promise.app.ZApplication;
import com.heitouyang.promise.retrofit.APIClient;
import com.heitouyang.promise.server.resp.base.RespBody;
import com.heitouyang.promise.util.Logger;
import com.heitouyang.promise.util.ZPreference;
import com.heitouyang.promise.util.ZToast;

import java.net.SocketTimeoutException;
import java.net.UnknownHostException;

import retrofit2.Call;
import retrofit2.Callback;
import retrofit2.Response;

/**

• Created by zw.zhang on 2017/8/17.
  */

public abstract class ZCallBackWithFail implements Callback {
/**
* override this method to do some thing after fail.
*/
public boolean fail;

public ZCallBackWithFail() {
}

public abstract void callBack(T response);

@Override
public void onResponse(@NonNull Call<T> call, @NonNull Response<T> response) {
    if ((response.code() != 200) || response.body() == null || (response.body().errcode != 0)) {
        fail = true;
    }
    if (response.code() >= 500) {
        fail = true;
        ZToast.toast("服务器暂时无法为您提供服务，请稍后再试");
    } else if (response.code() == 401) {
        loginOut("你需要重新登录");
        return;
    }

    try {
        if (response.body() != null && response.body().errcode != 0) {
            long errCode = response.body().errcode;
            if (errCode == -10000 || errCode == -10001 || errCode == -10006 || errCode == -10007 || errCode == -1012 || errCode == -1014) {
                String m = "你的账号已在其他设备登录，是否重新登录";
                if (errCode == -10001 || errCode == -10000) {
                    m = "登录过期，需要重新登录";
                } else if (errCode == -1012) {
                    m = "用户不存在";
                }
                loginOut(m);
            } else {
                ZToast.alertDialog(response.body().errmsg);
            }
        }
    } catch (Exception e) {
        e.printStackTrace();
    } finally {
        callBack(response.body());
    }
}

private void loginOut(String m) {
    if (ZPreference.isLogin()) {
        APIClient.getInstance().getApiInterface().loginOutNotRxjava().enqueue(new ZCallBackWithFail<RespBody>() {
            @Override
            public void callBack(RespBody response) {

                ZPreference.clear();
                Intent intent = new Intent("android.intent.action.MY_BROADCAST");
                intent.putExtra("msg", "hello receiver.");
                ZApplication.APP.get().sendBroadcast(intent);
                LocalBroadcastManager.getInstance(ZApplication.APP.get()).sendBroadcast(intent);
                ZToast.showLoginOutDialog(m, (dialog) -> ZActivityManager.getInstance().getCurrentActivity().startActivity(new Intent(ZApplication.APP.get(), LoginActivity.class)));
            }
        });
    }
}

@Override
public void onFailure(Call<T> call, Throwable t) {
    t.printStackTrace();
    Logger.e(t.getMessage());
    if (ZApplication.APP != null && ZApplication.APP.get() != null) {
        APIClient.initApiClient();
    }
    fail = true;

    if (t instanceof UnknownHostException || t instanceof SocketTimeoutException) {
        ZToast.toast("网络环境差，请检查网络设置");
    } else {
        ZToast.toast("服务器暂时无法为您提供服务，请稍后再试");
    }

    try {
        callBack(null);
    } catch (Exception e) {
        e.printStackTrace();
    }

}

}

[lbx2015]

I found "OkHttpClient: A connection to http://test-api.heitouyang.cn/ was leaked. Did you forget to close a response body?" should throw from glide. I do nothing about the glide OkHttpClient. How can I solve this with glide.

[lovelock]

It is wired. According to try-with-resources

The try-with-resources statement is a try statement that declares one or more resources. A resource is an object that must be closed after the program is finished with it. The try-with-resources statement ensures that each resource is closed at the end of the statement. Any object that implements java.lang.AutoCloseable, which includes all objects which implement java.io.Closeable, can be used as a resource.

public final class Response implements Closeable {
...

So I think this would work

        try (Response response = postCall(WRITE_LOG, payload)) {
            return true;
        }

because response would be closed by try-with-resources.
However tomcat warns that the connection was leaked.

[ssoper]

Fixing this in your code might be as simple as updating

return client.newCall(request).execute().code == 200

to

return client.newCall(request).execute().use {
    it.code == 200
}

[vibhorgarg]

1. we are using response.body().byteStream(), so we will have to manually close the body by calling response.close(); or response.body().close(); ?
2. on calling response.close(); , will the framework also terminate Http connection made to external service ?
   If yes, then it would lead to conection establishment overhead for every new request.

@JakeWharton @swankjesse