com.nimbusds.oauth2.sdk.http.HTTPRequest#closeStreams generates unnecessary java.net.ProtocolException "Cannot write output after reading input."

Issue #458 resolved
D Laurent created an issue

In Dynatrace, we are seeing a lot of supposedly “failed” requests in production.

These failed requests pertain to a java.net.ProtocolException with message "Cannot write output after reading input.".

This happens in sun.net.www.protocol.http.HttpURLConnection#getOutputStream0

in

// if there's already an input stream open, throw an exception
if (inputStream != null) {
    throw new ProtocolException("Cannot write output after reading input.");
}

which comes from client applications using the com.nimbusds.oauth2.sdk.http.HTTPRequest#send()

which at the end always closes the streams with com.nimbusds.oauth2.sdk.http.HTTPRequest#closeStreams

private static void closeStreams(final HttpURLConnection conn) {

    if (conn == null) {
       return;
    }

    try {
       if (conn.getInputStream() != null) {
          conn.getInputStream().close();
       }
    } catch (Exception e) {
       // ignore
    }

    try {
       if (conn.getOutputStream() != null) {
          conn.getOutputStream().close();
       }
    } catch (Exception e) {
       // ignore
    }

    try {
       if (conn.getErrorStream() != null) {
          conn.getOutputStream().close();
       }
    } catch (Exception e) {
       // ignore
    }
}

The issue with this strategy is that when you want to close the outputStream, you did not keep a reference to it and call again conn.getOutputStream() (at line 16 above) and this fails in sun.net.www.protocol.http.HttpURLConnection#getOutputStream0 because it refuses to give the reference to the outputStream after the inputStream has been initialized…

An improved way, which would not throw unnecessary exceptions, would be to keep the reference to the outputStream so that getOutputStream() does not have to be called again to close it.

Another way which can reduce the number of exception thrown would be to only close the outputStream when conn.getDoOutput() is true. This way would only throw the exception in case of “POST” and “PUT”.

Comments (7)

  1. Yavor Vasilev

    Hi and thanks for this detailed report. Would you be able to submit a pull request with the proposed changes?

  2. Yavor Vasilev

    It should work. But don’t worry, if there are issues posting the PR, feel free to post a patch here and we’ll do the rest.

    If there’s a way to lock the issue the the fix with a test, it would be really great.

  3. D Laurent reporter

    I attached a patch and a non-regression test, which makes the boilerplate kind-of ugly, since testing for a swallowed exception, detected only by profilers is not straightforward to test.

    What I did is add a debug flag in an additional package-private constructor.

    It could be alternatively a System property.

    Or one could use ByteBuddy in the test to change the closeStreams() method not to swallow exceptions, which would keep the code cleaner…

  4. Yavor Vasilev

    Thanks for the patch. It was applied here (moved the debug flag to a package private setter): 6ca683e

    Released now as v11.10.1.

    version 11.10.1 (2024-02-26)
        * Fixes an HTTPRequest.closeStreams bug that generated an unnecessary
          java.net.ProtocolException "Cannot write output after reading input."
          (iss #458).
    

  5. Log in to comment