Close Body after reading

[SenseiFisher]

readWithMessageConverters opens a FileInputStream but does not ensure it closes after reading from it.
Ensure the FileInputStream is closed.

Solves Issue:
#25896

[pivotal-issuemaster]

@SenseiFisher Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-issuemaster]

@SenseiFisher Thank you for signing the Contributor License Agreement!

[rstoyanchev]

I don't think it should be closed as it may not be the end of the response, for example in a multipart request but it could also be in another streaming scenario. See for example #14728 or the more recent regression #25989 caused by a similar fix.

Is there a specific issue that this presents?

[SenseiFisher]

Hi, I recived a "Too many opened files" (50,000) a few times at the production environment. After investigating it seems like the files belongs to tomcat. I ran some tests at our development environment and saw the files only closed at GC when using the StringHttpConverter (finalize). This led me to belive it just a matter of luck, if GCs are not executed frequent enough, every system will recive this exact same error. בתאריך יום ב׳, 2 בנוב׳ 2020, 15:44, מאת Rossen Stoyanchev ‏< [email protected]>:

…

I don't think it should be closed as it may not be the end of the response, for example in a multipart request but it could also be in another streaming scenario. See for example #14728 <#14728> or the more recent regression #25989 <#25989> caused by a similar fix. Is there a specific issue that this presents? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#25897 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKKQXHJYOKJ5B24MWTQSZTLSN2ZTDANCNFSM4SMVNCLQ> .

[jhoeller]

Since this is on the input side, we could potentially close it once the request body has been fully read. I'm less concerned there than on the output side but still wondering why it would be necessary.

Where does the FileInputStream come from, actually? Is it Tomcat opening such a stream?

[SenseiFisher]

Well... The InputSteam is created on the EmptyBodyCheckingHttpInputMessage constructor, and the instance is stored in "message". After some processing, the function ends and return the processed body. Making the "message" unreachable. As a result once the function ends the InputStream cannot be closed manually. בתאריך יום ג׳, 1 בדצמ׳ 2020, 10:12, מאת Juergen Hoeller ‏< [email protected]>:

…

Since this is on the input side, we could potentially close it once the request body has been fully read. I'm less concerned there than on the output side but still wondering why it would be necessary. Where does the FileInputStream come from, actually? Is it Tomcat opening such a stream? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#25897 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKKQXHJCJB3QLWYDTNIB5ITSSSQPRANCNFSM4SMVNCLQ> .

[rstoyanchev]

Scheduling for consideration in 6.0 M2 along with #27773.

Where does the FileInputStream come from, actually? Is it Tomcat opening such a stream?

If it is related to reading parts, as in #27773, then the the underlying input stream could be a temp file for the part.

[rstoyanchev]

We're going to address this in a similar way but with a slightly different implementation, so I'm closing this as superseded by #27773.