Search code examples
javabufferedreaderurlconnectiontry-with-resources

Proper use of URLConnection and try-with-resources in Java 8


I am not 100% if this code is 100% save against memory leaks or leaving sockets open:

    public static JSONObject readJsonFromUrl(String url) throws IOException, JSONException {

        URLConnection connection = new URL(url).openConnection();

        connection.setReadTimeout(5000);
        connection.setConnectTimeout(8000);

        try (InputStream is = connection.getInputStream()) {

            BufferedReader rd = new BufferedReader(new InputStreamReader(is, Charset.forName("UTF-8")));
            String jsonText = readAll(rd);

            JSONObject json = new JSONObject(jsonText);

            return json;
        }
    }

    private static String readAll(Reader rd) throws IOException {

        StringBuilder sb = new StringBuilder();

        int cp;

        while ((cp = rd.read()) != -1) {
            sb.append((char) cp);
        }

        return sb.toString();
    }

Would I need to put the "BufferedReader rd" into an inner second try with too, or would it be obsolete? What if a read or connection timeout occurs, and the try isn't finished yet? Why doesnt the URLConnection have a disconnect() or close() function btw?


Solution

  • All a Reader's close method really does, ultimately, is close resources. (See the javadoc.) Therefore, you don't really need to close the BufferedReader or InputStreamReader if you've closed the InputStream. However, it is more clear that you are closing all resources, and avoids compiler warnings, if you close it anyway. Since closing a Reader closes all underlying Readers and resources, closing the BufferedReader will close everything:

    try (BufferedReader rd = new BufferedReader(new InputStreamReader(
            connection.getInputStream()))) {
        return new JSONObject(readAll(rd));
    }
    

    So you only need to declare the top-level reader in your try-with-resources.