Bug 52723 - An incomplete fix for the resource leak bugs in StandardManager.java
An incomplete fix for the resource leak bugs in StandardManager.java
Status: RESOLVED WONTFIX
Product: Tomcat 6
Classification: Unclassified
Component: Catalina
unspecified
PC All
: P2 trivial (vote)
: default
Assigned To: Tomcat Developers Mailing List
:
Depends on:
Blocks:
  Show dependency tree
 
Reported: 2012-02-21 10:22 UTC by lianggt08
Modified: 2012-06-05 00:23 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description lianggt08 2012-02-21 10:22:02 UTC
The fix revision 907502 was aimed to remove an resource leak bug on the 
FileOutputStream object "fos" and the ObjectOutputStream "oos" in the method 
"doUnload" of the file 
"/tomcat/tc6.0.x/trunk/java/org/apache/catalina/session/StandardManager.java" , 
but it is incomplete. 

When the ObjectOutputStream object is created unsuccessfully but the temp 
BufferedOutputStream object is created successfully, the temp object will be 
leaked. 

Besides that, the "oos" is not closed in all the pathes.  The best way to close 
such resource object is putting such close operations in the finaly block of a 
try-catch-finally structure.
Comment 1 Mark Thomas 2012-03-09 20:35:31 UTC
Severity set to something more realistic given the likelihood of this ever
actually happening.

Fixed in trunk and 7.0.x for 7.0.27. Proposed for 6.0.x although I'd be equally
happy with WONTFIX for 6.0.x.
Comment 2 Konstantin Kolinko 2012-03-21 00:18:14 UTC
> When the ObjectOutputStream object is created unsuccessfully but the temp 
BufferedOutputStream object is created successfully, the temp object will be 
leaked.

BufferedOutputStream does not hold any native resources. Its close() method is mostly a noop. There is no need to call it explicitly in the scenario that you are describing.
Comment 3 Mark Thomas 2012-06-03 21:22:39 UTC
It isn't going to add any benefit so lets not change the 6.0.x code.
Comment 4 Konstantin Kolinko 2012-06-03 21:29:35 UTC
My comment 2 was wrong. I missed that the original code closes oos (which is always null there).  It should have closed the file stream instead.

oos constructor can throw IOException because it writes stream header (though due to buffering it is unlikely).
Comment 5 lianggt08 2012-06-04 10:36:48 UTC
Hi, Konstantin, can you explain more about your new comment (Comment 4)? 
Does the BufferedOutputStream temp object need to be closed in this code piece?
Comment 6 Konstantin Kolinko 2012-06-05 00:23:34 UTC
(In reply to comment #5)
1. BufferedOutputStream does not deserve closing, because it is in memory only. No native resources are leaking.

- ObjectOutputStream would never need to be closed. If an IOException happens  oos will be null as assignment never happens.

- The only one that really has to be closed is FileOutputStream.

- The only situation when FileOutputStream is not-null and needs to be closed is when its constructor succeeded, but constructors of other streams failed.

2. The only one of other constructors that can (in theory) fail with IOException is ObjectOutputStream.

It completes with IOException iff call to ObjectOutputStream#writeStreamHeader() throws an IOException.

My Comment 4 was caused by this point "2.".

Actually, looking further, the writeStreamHeader() method writes 4 bytes. As those 4 bytes are cached in BufferedOutputStream, an IOException cannot really happen there.

So sorry for the noise. It is not worth for 6.0.