You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by bu...@apache.org on 2010/01/31 01:53:05 UTC
DO NOT REPLY [Bug 48643] New: catalina.session.FileStore.save() and
load() - close() bugs
https://issues.apache.org/bugzilla/show_bug.cgi?id=48643
Summary: catalina.session.FileStore.save() and load() - close()
bugs
Product: Tomcat 7
Version: trunk
Platform: PC
OS/Version: Windows XP
Status: NEW
Severity: normal
Priority: P2
Component: Catalina
AssignedTo: dev@tomcat.apache.org
ReportedBy: sebb@apache.org
The code for the save() method reads:
try {
fos = new FileOutputStream(file.getAbsolutePath());
oos = new ObjectOutputStream(new BufferedOutputStream(fos));
} catch (IOException e) {
if (oos != null) { <== oos can only be null here
try {
oos.close();
} catch (IOException f) {
// Ignore
}
}
throw e;
}
Presumably the catch statement should try to close fos instead?
Similarly for the load() method; at line 280/281 there is the code:
} catch (IOException e) {
if (ois != null) {
However, ois is the last object created by the try clause, so must be null.
Probably the code should check and close bis and/or fis.
At the end of the load() method, the code says:
// Close the input stream
if (ois != null) {
However, ois cannot be null at that point.
Note: these bugs were detected by the Eclipse compiler.
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
DO NOT REPLY [Bug 48643] catalina.session.FileStore.save() and
load() - close() bugs
Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=48643
--- Comment #1 from Sebb <se...@apache.org> 2010-01-30 17:10:36 UTC ---
There are similar problems in session.StandardManager.doUnload():
line 502, 530, and at line 542 there's a block of code:
// Flush and close the output stream
try {
oos.flush();
oos.close();
oos = null;
} catch (IOException e) {
if (oos != null) {
try {
oos.close();
} catch (IOException f) {
// Ignore
}
oos = null;
}
throw e;
}
This code looks as though it is trying to close() oos if the flush() fails, but
it will also try to close() oos if the close() fails.
Probably the first oos.close() should be removed.
AFAICT, there is also no need to set the oos field to null.
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
DO NOT REPLY [Bug 48643] catalina.session.FileStore.save() and
load() - close() bugs
Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=48643
Mark Thomas <ma...@apache.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution| |FIXED
--- Comment #4 from Mark Thomas <ma...@apache.org> 2010-02-07 14:16:50 UTC ---
Fix in trunk for 7.0.x
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
DO NOT REPLY [Bug 48643] catalina.session.FileStore.save() and
load() - close() bugs
Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=48643
Konstantin Kolinko <kn...@gmail.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Severity|normal |minor
--- Comment #2 from Konstantin Kolinko <kn...@gmail.com> 2010-01-30 17:49:39 UTC ---
(In reply to comment #0)
> Note: these bugs were detected by the Eclipse compiler.
I think it is pointless to try to please Eclipse IDE here. That "possible null
access check" is an optional feature (off by default), depends on the version
of IDE that you are using, and also buggy at times, as I experiences just
recently (it gave me contradictory tips regarding "unnecessary null check" for
one line, and then "possible null pointer access" for the same variable few
lines later). Then some other tool will start complaining that we have not
closed a stream there...
Back to the code: I'll agree that you are right that first oos.close(); is
actually never called, but I do not think we need to close fos here.
I do not see what can fail in the "new ObjectOutputStream(new
BufferedOutputStream(fos))" line, besides maybe an OutOfMemoryError.
As of now, the code is easier to read, even if it is redundant.
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
DO NOT REPLY [Bug 48643] catalina.session.FileStore.save() and
load() - close() bugs
Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=48643
--- Comment #3 from Sebb <se...@apache.org> 2010-01-31 03:23:42 UTC ---
(In reply to comment #2)
> (In reply to comment #0)
> > Note: these bugs were detected by the Eclipse compiler.
>
> I think it is pointless to try to please Eclipse IDE here. That "possible null
> access check" is an optional feature (off by default), depends on the version
> of IDE that you are using, and also buggy at times
It's not a question of trying to 'please' Eclipse.
It has reported a possible error; in this case manual inspection shows that it
was correct to do so.
> Back to the code: I'll agree that you are right that first oos.close(); is
> actually never called, but I do not think we need to close fos here.
In which case I think the try catch block can be removed entirely.
> I do not see what can fail in the "new ObjectOutputStream(new
> BufferedOutputStream(fos))" line, besides maybe an OutOfMemoryError.
Agreed. So why try to close oos?
> As of now, the code is easier to read, even if it is redundant.
Disagree - it's confusing to have code that cannot be executed.
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org