Connection not closed in DefaultResourceRetriever

[Olivier Meurice]

In method com.nimbusds.jose.util.DefaultResourceRetriever.retrieveResource(URL url) the URLConnection is not closed.

[Thierry De Leeuw]

Here is a proposed fix. Note that the same code is present in other repos. Some people like to wrap the "con.close()" in a try catch and do not rethrow the closing exception, but just log it to make sure that it does not hide a potential exception raised in the body of the method. Also note that the unit test fail on my machine (commit 0c9bd531358cd04fdd075550ee89e90a1af4e7ab).

    @Override
    public Resource retrieveResource(final URL url)
            throws IOException {

        HttpURLConnection con = null;
        final String content;
        try {
            con = (HttpURLConnection) url.openConnection();
            con.setConnectTimeout(getConnectTimeout());
            con.setReadTimeout(getReadTimeout());

            InputStream inputStream = con.getInputStream();
            try {
                if (getSizeLimit() > 0) {
                    inputStream = new BoundedInputStream(inputStream, getSizeLimit());
                }

                content = IOUtils.readInputStreamToString(inputStream, Charset.forName("UTF-8"));
            } finally {
                inputStream.close();
            }

            // Check HTTP code + message
            final int statusCode = con.getResponseCode();
            final String statusMessage = con.getResponseMessage();

            // Ensure 2xx status code
            if (statusCode > 299 || statusCode < 200) {
                throw new IOException("HTTP " + statusCode + ": " + statusMessage);
            }

            return new Resource(content, con.getContentType());
        } catch (ClassCastException e) {
            throw new IOException("Couldn't open HTTP(S) connection: " + e.getMessage(), e);
        } finally {
            if (con != null) {
                con.disconnect();
            }
        }
    }

[Connect2id OSS]

Hi,

I just went through your issue which you originally had posted in the OIDC SDK, and now I see that you actually mean closing the HTTP TCP socket, not the input stream.

Could you please detail which unit test is failing on your machine?

The HTTP socket is left unclosed so the JVM may reuse it for resources that can benefit from "keep alive":

https://techblog.bozho.net/caveats-of-httpurlconnection/

If your use case requires the socket to be closed immediately after use, I guess adding a new constructor with the option to disconnect after use will do.

[Thierry De Leeuw]

Hi Indeed it looks like the connection is reused. Note that the javadoc mentioned in this article indicates that "Disconnect. Once the response body has been read, the HttpURLConnection should be closed by calling disconnect(). Disconnecting releases the resources held by a connection so they may be closed or reused" - every example they give embed the disconnect call in a finally block. Testing with Oracle JDK 1.8_152, the connection is reused even after an explicit disconect() call. We encounetered a connection timeout in this code during our stress tests, which made me (wronlgy) believe that not closing the connection explicitly might have consumed our connection pool on the server. Best regards Thierry

[Connect2id OSS]

Thanks for sharing your observation with Oracle JDK 1.8_152.

I found out that the "Disconnect. Once ..." comes from the Android documentation: http://www.mit.edu/afs.new/sipb/project/android/docs/reference/java/net/HttpURLConnection.html

As for the JDK, between JDK 5,6,7,8 and 9 the documentation remains identical:

https://docs.oracle.com/javase/8/docs/api/java/net/HttpURLConnection.html

Calling the close() methods on the InputStream or OutputStream of an HttpURLConnection after a request may free network resources associated with this instance but has no effect on any shared persistent connection. Calling the disconnect() method may close the underlying socket if a persistent connection is otherwise idle at that time.

https://docs.oracle.com/javase/8/docs/api/java/net/HttpURLConnection.html#disconnect--

Indicates that other requests to the server are unlikely in the near future. Calling disconnect() should not imply that this HttpURLConnection instance can be reused for other requests.

The latter JavaDoc seems to contradict what you've actually observed.

[Connect2id OSS]

Modifies DefaultResourceRetriever to disconnect by default : 0f2617f

(also adds new constructor and setter to modify disconnect call behaviour)

[Connect2id OSS]

Now out http://search.maven.org/#artifactdetails|com.nimbusds|nimbus-jose-jwt|5.3|jar