com.nimbusds.oauth2.sdk.http.HTTPRequest#closeStreams generates unnecessary java.net.ProtocolException "Cannot write output after reading input."
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)
-
-
reporter Hi,
Sure! I don’t think I have the right to create a PR though. -
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.
-
reporter - attached PR-458__Preventing__java_net_ProtocolException__Cannot_write_output_after_reading_input___.patch
<div class="preview-container wiki-content"><!-- loaded via ajax --></div> <div class="mask"></div> </div>
</div> </form>
-
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…
-
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).
-
- changed status to resolved
- Log in to comment
Hi and thanks for this detailed report. Would you be able to submit a pull request with the proposed changes?