Skip to main content

Review: Issue #27

4 replies [Last post]
Anonymous

Vladimir

Could you please review a bugfix for
Issue #27: Possible resources leak in finally blocks

Thanks
--
Oleg

/opt/csw/bin/svn diff -r272:273
https://cqme.dev.java.net/svn/cqme/branches/users/modelka/Issue27
Index:
unitTests/classes/com/sun/tck/j2me/services/messagingService/tests/MessageTests.java
===================================================================
---
unitTests/classes/com/sun/tck/j2me/services/messagingService/tests/MessageTests.java
(revision 272)
+++
unitTests/classes/com/sun/tck/j2me/services/messagingService/tests/MessageTests.java
(revision 273)
@@ -155,10 +155,18 @@
assertEquals(msg_NullData, Message.read(dis));
} finally {
if (dos != null) {
- dos.close();
+ try {
+ dos.close();
+ } catch (IOException e) {
+ // ignore
+ }
}
if (dis != null) {
- dis.close();
+ try {
+ dis.close();
+ } catch (IOException e) {
+ // ignore
+ }
}
}
}
Index: src/share/classes/com/sun/tck/cldc/javatest/TestBuilder.java
===================================================================
--- src/share/classes/com/sun/tck/cldc/javatest/TestBuilder.java
(revision 272)
+++ src/share/classes/com/sun/tck/cldc/javatest/TestBuilder.java
(revision 273)
@@ -136,20 +136,20 @@
if (!fCheck.canRead()) {
throw new RuntimeException("TestBuilder: Cannot read
jar file: " + fCheck.getAbsolutePath());
}
- JarFile f = null;
+ JarFile jarFile = null;
try {
- f = new JarFile(fname);
- Enumeration es = f.entries();
+ jarFile = new JarFile(fname);
+ Enumeration es = jarFile.entries();
while (es.hasMoreElements()) {
- JarEntry e = (JarEntry) es.nextElement();
- if (e.getName().startsWith("META-INF")) {
+ JarEntry jarEntry = (JarEntry) es.nextElement();
+ if (jarEntry.getName().startsWith("META-INF")) {
continue;
}
- data = new byte[(int) e.getSize()];
+ data = new byte[(int) jarEntry.getSize()];

InputStream is = null;
try {
- is = f.getInputStream(e);
+ is = jarFile.getInputStream(jarEntry);
int chunk = 0;
int readed = 0;
while ((readed < data.length) && (chunk >= 0)) {
@@ -160,17 +160,21 @@
throw new RuntimeException("TestBuilder: " +
ioe.toString());
} finally {
if (is != null) {
- is.close();
+ try {
+ is.close();
+ } catch (IOException e) {
+ e.printStackTrace();
+ }
}
}
- files.put(e.getName(), data);
+ files.put(jarEntry.getName(), data);
}
} catch (IOException e) {
throw new RuntimeException("TestBuilder: " + e.toString());
} finally {
- if (f != null) {
+ if (jarFile != null) {
try {
- f.close();
+ jarFile.close();
} catch (Exception e) {
// Do nothing
}
Index: src/share/classes/com/sun/tck/midp/lib/OTASupportServer.java
===================================================================
--- src/share/classes/com/sun/tck/midp/lib/OTASupportServer.java
(revision 272)
+++ src/share/classes/com/sun/tck/midp/lib/OTASupportServer.java
(revision 273)
@@ -748,12 +748,20 @@
sendDiagnostics(HTTP_BAD_REQUEST);
return;
} finally {
- try {
- dis.close();
- bais.close();
- } catch (IOException ioe) {
- // ignore, can't do much
+ if (dis != null) {
+ try {
+ dis.close();
+ } catch (IOException e) {
+ // ignore
+ }
}
+ if (bais != null) {
+ try {
+ bais.close();
+ } catch (IOException ioe) {
+ // ignore
+ }
+ }
}

sendDiagnostics(HTTP_OK);
Index:
src/share/classes/com/sun/tck/j2me/services/commService/CommToJavatestService.java
===================================================================
---
src/share/classes/com/sun/tck/j2me/services/commService/CommToJavatestService.java
(revision 272)
+++
src/share/classes/com/sun/tck/j2me/services/commService/CommToJavatestService.java
(revision 273)
@@ -167,7 +167,11 @@
}
} finally {
if (dos != null) {
- dos.close();
+ try {
+ dos.close();
+ } catch (IOException e) {
+ e.printStackTrace();
+ }
}
}
}
@@ -221,7 +225,11 @@
}
} finally {
if (dos != null) {
- dos.close();
+ try {
+ dos.close();
+ } catch (IOException e) {
+ e.printStackTrace();
+ }
}
}
}
@@ -270,7 +278,11 @@
dos.write(data);
} finally {
if (dos != null) {
- dos.close();
+ try {
+ dos.close();
+ } catch (IOException e) {
+ e.printStackTrace();
+ }
}
}

@@ -297,7 +309,11 @@
dos.writeInt(i);
} finally {
if (dos != null) {
- dos.close();
+ try {
+ dos.close();
+ } catch (IOException e) {
+ e.printStackTrace();
+ }
}
}
byte[] resp = base.send(baos.toByteArray()); // send
request for the next portion
@@ -357,7 +373,11 @@
dis.readFully(respPacket);
} finally {
if (dis != null) {
- dis.close();
+ try {
+ dis.close();
+ } catch (IOException e) {
+ e.printStackTrace();
+ }
}
}
}
Index:
src/share/classes/com/sun/tck/j2me/services/commService/clients/CldcHTTPCommClient.java
===================================================================
---
src/share/classes/com/sun/tck/j2me/services/commService/clients/CldcHTTPCommClient.java
(revision 272)
+++
src/share/classes/com/sun/tck/j2me/services/commService/clients/CldcHTTPCommClient.java
(revision 273)
@@ -114,19 +114,23 @@
if (is != null) {
try {
is.close();
- } catch (IOException e1) {
+ } catch (IOException e) {
// Silently ignore
}
}
if (os != null) {
try {
os.close();
- } catch (IOException e1) {
+ } catch (IOException e) {
// Silently ignore
}
}
if (hc != null) {
- hc.close();
+ try {
+ hc.close();
+ } catch (IOException e) {
+ e.printStackTrace();
+ }
}
}
}
Index:
src/share/classes/com/sun/cldc/communication/midp/MIDHttpExecutionServer.java
===================================================================
---
src/share/classes/com/sun/cldc/communication/midp/MIDHttpExecutionServer.java
(revision 272)
+++
src/share/classes/com/sun/cldc/communication/midp/MIDHttpExecutionServer.java
(revision 273)
@@ -280,7 +280,11 @@
fos.flush();
} finally {
if (fos != null) {
- fos.close();
+ try {
+ fos.close();
+ } catch (IOException e) {
+ e.printStackTrace();
+ }
}
}
}
@@ -530,13 +534,16 @@
File file = new File(serverRoot, path);
if (file.isFile() && file.canRead()) {
try {
- FileInputStream fis = new FileInputStream(file);
- DataInputStream fin = new DataInputStream(fis);
+ DataInputStream fin = new DataInputStream(new
FileInputStream(file));
byte[] buf = new byte[(int) file.length()];
try {
fin.readFully(buf);
} finally {
- fin.close();
+ try {
+ fin.close();
+ } catch (IOException e) {
+ e.printStackTrace();
+ }
}
setResponseProperty("Content-Length", "" +
file.length());
setResponseProperty("Content-Type",
Index: src/share/classes/com/sun/cldc/communication/midp/MIDHttpClient.java
===================================================================
---
src/share/classes/com/sun/cldc/communication/midp/MIDHttpClient.java
(revision 272)
+++
src/share/classes/com/sun/cldc/communication/midp/MIDHttpClient.java
(revision 273)
@@ -79,18 +79,21 @@
} catch (IOException e) {
throw new RuntimeException ("HttpClient: Unexpected
IOException while " +
"reading getNextTest - " + e);
- } finally {
- try {
- if (is != null) {
- is.close();
- }
- if (hc != null) {
- hc.close();
- }
- } catch (IOException e) {
- throw new RuntimeException ("HttpClient: Unexpected
IOException " +
- "while closing
communication - " + e);
+ } finally {
+ if (is != null) {
+ try {
+ is.close();
+ } catch (IOException e) {
+ e.printStackTrace();
+ }
}
+ if (hc != null) {
+ try {
+ hc.close();
+ } catch (IOException e) {
+ e.printStackTrace();
+ }
+ }
}
}

@@ -101,7 +104,7 @@
try {
String dest = stream_url + "sendTestResult" + BundleID;
trace("Opening connection to " + dest);
- hc = (HttpConnection)Connector.open(dest,
Connector.READ_WRITE, false);
+ hc = (HttpConnection) Connector.open(dest,
Connector.READ_WRITE, false);
hc.setRequestMethod("POST");
hc.setRequestProperty("Connection", "close");
trace("Posting data..");
@@ -118,17 +121,20 @@
throw new RuntimeException ("HttpClient: Unexpected
IOException while " +
"writing sendTestResult - " + e);
} finally {
- try {
- if (os != null) {
- os.close();
- }
- if (hc != null) {
- hc.close();
- }
- } catch (IOException e) {
- throw new RuntimeException ("HttpClient: Unexpected
IOException " +
- "while closing
communication - " + e);
+ if (os != null) {
+ try {
+ os.close();
+ } catch (IOException e) {
+ e.printStackTrace();
+ }
}
+ if (hc != null) {
+ try {
+ hc.close();
+ } catch (IOException e) {
+ e.printStackTrace();
+ }
+ }
}
}

Index:
src/share/classes/com/sun/cldc/communication/midp/DefaultContentHandler.java
===================================================================
---
src/share/classes/com/sun/cldc/communication/midp/DefaultContentHandler.java
(revision 272)
+++
src/share/classes/com/sun/cldc/communication/midp/DefaultContentHandler.java
(revision 273)
@@ -65,7 +65,11 @@
try {
fin.readFully(buf);
} finally {
- fin.close();
+ try {
+ fin.close();
+ } catch (IOException e) {
+ e.printStackTrace();
+ }
}
server.setResponseProperty("Content-Length",
"" + jarFile.length());

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Reply viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.
Vladimir Sizikov

Hi Oleg,

Thanks for the contribution!

Let's first discuss two general cases, mentioned in the bug report.

1. Resource leaks.

Yes, there are places in the current code where mulpiple streams are
being closed in a sequence, and any IOE from close() would lead to
situation that some other streams/resources might not be cleaned.

I agree that this is a problem and think we should take care of this
situation and properly close the streams.

2.
The bug also mentions the following code:

try {
fin.readFully(buf);
} finally {
fin.close();
}

I don't see anythirg wrong with it. IOEexception, thrown out of the
close() will propagate up the stack and be dealt with at the
appropriate level. There are no resource leaks in tihs case.
Not to mention the fact that in most cases the IOE will _never_ be
thrown due to fact that ByteArray streams are used and their close()
is a noop.

In general, I think it's better to let the exception propagate then to
hide with:

} catch (IOException e) {
e.printStackTrace();
}

In fact, hiding the exception behind e.printStackTrace() is a know anti-pattern

So, having said all of the above, let's focus on the actual cases when
resource leaks can happen.

There are 3 such cases with possible resource leaks I see:

unitTests/classes/com/sun/tck/j2me/services/messagingService/tests/MessageTests.java

The change is OK, but I don't like the we completely hide the
IOE. Yes, it is not going to happen anyways, but if it's going to
happen on some broken implementation, we'll never know.

Why not replace the two sequential close() calls to somithing like
this:

try {
if (dos != null) {
dos.close();
}
} finally {
if (dis != null) {
dis.close();
}
}

and let the IOE propagate up the stack, in that rare (if ever
possible) case when IOE is thrown.

src/share/classes/com/sun/tck/midp/lib/OTASupportServer.java

The 'dis' and 'bais' cannot be null, so there is need to check for
null here.

Again, the subsequent invocations:
dis.close();
bais.close();

can be replaced by much simple alternative:

try {
dis.close();
finally {
bais.close();
}

src/share/classes/com/sun/cldc/communication/midp/MIDHttpClient.java

Similar to the first case, there is no need to change the original
code much (and definitely there is need to eliminante user-friendly
message.

The rest of the changes mostly relate to hiding the IOException, and I
don't think we should do that.

Thanks,
--Vladimir

On Sun, Jan 28, 2007 at 11:16:41PM +0300, Oleg Vazhnev wrote:
> Vladimir
>
> Could you please review a bugfix for
> Issue #27: Possible resources leak in finally blocks
>
> Thanks
> --
> Oleg
>
> /opt/csw/bin/svn diff -r272:273
> https://cqme.dev.java.net/svn/cqme/branches/users/modelka/Issue27
> Index:
> unitTests/classes/com/sun/tck/j2me/services/messagingService/tests/MessageTests.java
> ===================================================================
> ---
> unitTests/classes/com/sun/tck/j2me/services/messagingService/tests/MessageTests.java
> (revision 272)
> +++
> unitTests/classes/com/sun/tck/j2me/services/messagingService/tests/MessageTests.java
> (revision 273)
> @@ -155,10 +155,18 @@
> assertEquals(msg_NullData, Message.read(dis));
> } finally {
> if (dos != null) {
> - dos.close();
> + try {
> + dos.close();
> + } catch (IOException e) {
> + // ignore
> + }
> }
> if (dis != null) {
> - dis.close();
> + try {
> + dis.close();
> + } catch (IOException e) {
> + // ignore
> + }
> }
> }
> }
> Index: src/share/classes/com/sun/tck/cldc/javatest/TestBuilder.java
> ===================================================================
> --- src/share/classes/com/sun/tck/cldc/javatest/TestBuilder.java
> (revision 272)
> +++ src/share/classes/com/sun/tck/cldc/javatest/TestBuilder.java
> (revision 273)
> @@ -136,20 +136,20 @@
> if (!fCheck.canRead()) {
> throw new RuntimeException("TestBuilder: Cannot read
> jar file: " + fCheck.getAbsolutePath());
> }
> - JarFile f = null;
> + JarFile jarFile = null;
> try {
> - f = new JarFile(fname);
> - Enumeration es = f.entries();
> + jarFile = new JarFile(fname);
> + Enumeration es = jarFile.entries();
> while (es.hasMoreElements()) {
> - JarEntry e = (JarEntry) es.nextElement();
> - if (e.getName().startsWith("META-INF")) {
> + JarEntry jarEntry = (JarEntry) es.nextElement();
> + if (jarEntry.getName().startsWith("META-INF")) {
> continue;
> }
> - data = new byte[(int) e.getSize()];
> + data = new byte[(int) jarEntry.getSize()];
>
> InputStream is = null;
> try {
> - is = f.getInputStream(e);
> + is = jarFile.getInputStream(jarEntry);
> int chunk = 0;
> int readed = 0;
> while ((readed < data.length) && (chunk >= 0)) {
> @@ -160,17 +160,21 @@
> throw new RuntimeException("TestBuilder: " +
> ioe.toString());
> } finally {
> if (is != null) {
> - is.close();
> + try {
> + is.close();
> + } catch (IOException e) {
> + e.printStackTrace();
> + }
> }
> }
> - files.put(e.getName(), data);
> + files.put(jarEntry.getName(), data);
> }
> } catch (IOException e) {
> throw new RuntimeException("TestBuilder: " + e.toString());
> } finally {
> - if (f != null) {
> + if (jarFile != null) {
> try {
> - f.close();
> + jarFile.close();
> } catch (Exception e) {
> // Do nothing
> }
> Index: src/share/classes/com/sun/tck/midp/lib/OTASupportServer.java
> ===================================================================
> --- src/share/classes/com/sun/tck/midp/lib/OTASupportServer.java
> (revision 272)
> +++ src/share/classes/com/sun/tck/midp/lib/OTASupportServer.java
> (revision 273)
> @@ -748,12 +748,20 @@
> sendDiagnostics(HTTP_BAD_REQUEST);
> return;
> } finally {
> - try {
> - dis.close();
> - bais.close();
> - } catch (IOException ioe) {
> - // ignore, can't do much
> + if (dis != null) {
> + try {
> + dis.close();
> + } catch (IOException e) {
> + // ignore
> + }
> }
> + if (bais != null) {
> + try {
> + bais.close();
> + } catch (IOException ioe) {
> + // ignore
> + }
> + }
> }
>
> sendDiagnostics(HTTP_OK);
> Index:
> src/share/classes/com/sun/tck/j2me/services/commService/CommToJavatestService.java
> ===================================================================
> ---
> src/share/classes/com/sun/tck/j2me/services/commService/CommToJavatestService.java
> (revision 272)
> +++
> src/share/classes/com/sun/tck/j2me/services/commService/CommToJavatestService.java
> (revision 273)
> @@ -167,7 +167,11 @@
> }
> } finally {
> if (dos != null) {
> - dos.close();
> + try {
> + dos.close();
> + } catch (IOException e) {
> + e.printStackTrace();
> + }
> }
> }
> }
> @@ -221,7 +225,11 @@
> }
> } finally {
> if (dos != null) {
> - dos.close();
> + try {
> + dos.close();
> + } catch (IOException e) {
> + e.printStackTrace();
> + }
> }
> }
> }
> @@ -270,7 +278,11 @@
> dos.write(data);
> } finally {
> if (dos != null) {
> - dos.close();
> + try {
> + dos.close();
> + } catch (IOException e) {
> + e.printStackTrace();
> + }
> }
> }
>
> @@ -297,7 +309,11 @@
> dos.writeInt(i);
> } finally {
> if (dos != null) {
> - dos.close();
> + try {
> + dos.close();
> + } catch (IOException e) {
> + e.printStackTrace();
> + }
> }
> }
> byte[] resp = base.send(baos.toByteArray()); // send
> request for the next portion
> @@ -357,7 +373,11 @@
> dis.readFully(respPacket);
> } finally {
> if (dis != null) {
> - dis.close();
> + try {
> + dis.close();
> + } catch (IOException e) {
> + e.printStackTrace();
> + }
> }
> }
> }
> Index:
> src/share/classes/com/sun/tck/j2me/services/commService/clients/CldcHTTPCommClient.java
> ===================================================================
> ---
> src/share/classes/com/sun/tck/j2me/services/commService/clients/CldcHTTPCommClient.java
> (revision 272)
> +++
> src/share/classes/com/sun/tck/j2me/services/commService/clients/CldcHTTPCommClient.java
> (revision 273)
> @@ -114,19 +114,23 @@
> if (is != null) {
> try {
> is.close();
> - } catch (IOException e1) {
> + } catch (IOException e) {
> // Silently ignore
> }
> }
> if (os != null) {
> try {
> os.close();
> - } catch (IOException e1) {
> + } catch (IOException e) {
> // Silently ignore
> }
> }
> if (hc != null) {
> - hc.close();
> + try {
> + hc.close();
> + } catch (IOException e) {
> + e.printStackTrace();
> + }
> }
> }
> }
> Index:
> src/share/classes/com/sun/cldc/communication/midp/MIDHttpExecutionServer.java
> ===================================================================
> ---
> src/share/classes/com/sun/cldc/communication/midp/MIDHttpExecutionServer.java
> (revision 272)
> +++
> src/share/classes/com/sun/cldc/communication/midp/MIDHttpExecutionServer.java
> (revision 273)
> @@ -280,7 +280,11 @@
> fos.flush();
> } finally {
> if (fos != null) {
> - fos.close();
> + try {
> + fos.close();
> + } catch (IOException e) {
> + e.printStackTrace();
> + }
> }
> }
> }
> @@ -530,13 +534,16 @@
> File file = new File(serverRoot, path);
> if (file.isFile() && file.canRead()) {
> try {
> - FileInputStream fis = new FileInputStream(file);
> - DataInputStream fin = new DataInputStream(fis);
> + DataInputStream fin = new DataInputStream(new
> FileInputStream(file));
> byte[] buf = new byte[(int) file.length()];
> try {
> fin.readFully(buf);
> } finally {
> - fin.close();
> + try {
> + fin.close();
> + } catch (IOException e) {
> + e.printStackTrace();
> + }
> }
> setResponseProperty("Content-Length", "" +
> file.length());
> setResponseProperty("Content-Type",
> Index: src/share/classes/com/sun/cldc/communication/midp/MIDHttpClient.java
> ===================================================================
> ---
> src/share/classes/com/sun/cldc/communication/midp/MIDHttpClient.java
> (revision 272)
> +++
> src/share/classes/com/sun/cldc/communication/midp/MIDHttpClient.java
> (revision 273)
> @@ -79,18 +79,21 @@
> } catch (IOException e) {
> throw new RuntimeException ("HttpClient: Unexpected
> IOException while " +
> "reading getNextTest - " + e);
> - } finally {
> - try {
> - if (is != null) {
> - is.close();
> - }
> - if (hc != null) {
> - hc.close();
> - }
> - } catch (IOException e) {
> - throw new RuntimeException ("HttpClient: Unexpected
> IOException " +
> - "while closing
> communication - " + e);
> + } finally {
> + if (is != null) {
> + try {
> + is.close();
> + } catch (IOException e) {
> + e.printStackTrace();
> + }
> }
> + if (hc != null) {
> + try {
> + hc.close();
> + } catch (IOException e) {
> + e.printStackTrace();
> + }
> + }
> }
> }
>
> @@ -101,7 +104,7 @@
> try {
> String dest = stream_url + "sendTestResult" + BundleID;
> trace("Opening connection to " + dest);
> - hc = (HttpConnection)Connector.open(dest,
> Connector.READ_WRITE, false);
> + hc = (HttpConnection) Connector.open(dest,
> Connector.READ_WRITE, false);
> hc.setRequestMethod("POST");
> hc.setRequestProperty("Connection", "close");
> trace("Posting data..");
> @@ -118,17 +121,20 @@
> throw new RuntimeException ("HttpClient: Unexpected
> IOException while " +
> "writing sendTestResult - " + e);
> } finally {
> - try {
> - if (os != null) {
> - os.close();
> - }
> - if (hc != null) {
> - hc.close();
> - }
> - } catch (IOException e) {
> - throw new RuntimeException ("HttpClient: Unexpected
> IOException " +
> - "while closing
> communication - " + e);
> + if (os != null) {
> + try {
> + os.close();
> + } catch (IOException e) {
> + e.printStackTrace();
> + }
> }
> + if (hc != null) {
> + try {
> + hc.close();
> + } catch (IOException e) {
> + e.printStackTrace();
> + }
> + }
> }
> }
>
> Index:
> src/share/classes/com/sun/cldc/communication/midp/DefaultContentHandler.java
> ===================================================================
> ---
> src/share/classes/com/sun/cldc/communication/midp/DefaultContentHandler.java
> (revision 272)
> +++
> src/share/classes/com/sun/cldc/communication/midp/DefaultContentHandler.java
> (revision 273)
> @@ -65,7 +65,11 @@
> try {
> fin.readFully(buf);
> } finally {
> - fin.close();
> + try {
> + fin.close();
> + } catch (IOException e) {
> + e.printStackTrace();
> + }
> }
> server.setResponseProperty("Content-Length",
> "" + jarFile.length());
>

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Vladimir Sizikov

Hi,

Small clarification below.

On Mon, Jan 29, 2007 at 03:24:44AM -0800, Vladimir Sizikov wrote:
> 2.
> The bug also mentions the following code:
>
> try {
> fin.readFully(buf);
> } finally {
> fin.close();
> }
>
> I don't see anythirg wrong with it. IOEexception, thrown out of the
> close() will propagate up the stack and be dealt with at the
> appropriate level. There are no resource leaks in tihs case.

Well, to be absolutely correct, there is a small issue with
possibility of the second IOE to hide the original one.
But in our case I don't think that the problem is very real, and I
don't think that it justifies the updates (and possibility to
introduce regressions, etc).

You see, in the case above, both exceptions are very similar and deal
with the same stream and both mean that "something is wrong with the
stream". So, there should no confusion and problems when investigating
the exceptions.
Also, exceptions out of close() are typically _very_ rare.

I think it's better to leave this as is, and only fix the actual
possible resource leaks.

Thanks,
--Vladimir

> Not to mention the fact that in most cases the IOE will _never_ be
> thrown due to fact that ByteArray streams are used and their close()
> is a noop.
>
> In general, I think it's better to let the exception propagate then to
> hide with:
>
> } catch (IOException e) {
> e.printStackTrace();
> }
>
> In fact, hiding the exception behind e.printStackTrace() is a know anti-pattern
>
> So, having said all of the above, let's focus on the actual cases when
> resource leaks can happen.
>
> There are 3 such cases with possible resource leaks I see:
>
> unitTests/classes/com/sun/tck/j2me/services/messagingService/tests/MessageTests.java
>
> The change is OK, but I don't like the we completely hide the
> IOE. Yes, it is not going to happen anyways, but if it's going to
> happen on some broken implementation, we'll never know.
>
> Why not replace the two sequential close() calls to somithing like
> this:
>
> try {
> if (dos != null) {
> dos.close();
> }
> } finally {
> if (dis != null) {
> dis.close();
> }
> }
>
> and let the IOE propagate up the stack, in that rare (if ever
> possible) case when IOE is thrown.
>
> src/share/classes/com/sun/tck/midp/lib/OTASupportServer.java
>
> The 'dis' and 'bais' cannot be null, so there is need to check for
> null here.
>
> Again, the subsequent invocations:
> dis.close();
> bais.close();
>
> can be replaced by much simple alternative:
>
> try {
> dis.close();
> finally {
> bais.close();
> }
>
> src/share/classes/com/sun/cldc/communication/midp/MIDHttpClient.java
>
> Similar to the first case, there is no need to change the original
> code much (and definitely there is need to eliminante user-friendly
> message.
>
> The rest of the changes mostly relate to hiding the IOException, and I
> don't think we should do that.
>
> Thanks,
> --Vladimir
>
>
> On Sun, Jan 28, 2007 at 11:16:41PM +0300, Oleg Vazhnev wrote:
> > Vladimir
> >
> > Could you please review a bugfix for
> > Issue #27: Possible resources leak in finally blocks
> >
> > Thanks
> > --
> > Oleg
> >
> > /opt/csw/bin/svn diff -r272:273
> > https://cqme.dev.java.net/svn/cqme/branches/users/modelka/Issue27
> > Index:
> > unitTests/classes/com/sun/tck/j2me/services/messagingService/tests/MessageTests.java
> > ===================================================================
> > ---
> > unitTests/classes/com/sun/tck/j2me/services/messagingService/tests/MessageTests.java
> > (revision 272)
> > +++
> > unitTests/classes/com/sun/tck/j2me/services/messagingService/tests/MessageTests.java
> > (revision 273)
> > @@ -155,10 +155,18 @@
> > assertEquals(msg_NullData, Message.read(dis));
> > } finally {
> > if (dos != null) {
> > - dos.close();
> > + try {
> > + dos.close();
> > + } catch (IOException e) {
> > + // ignore
> > + }
> > }
> > if (dis != null) {
> > - dis.close();
> > + try {
> > + dis.close();
> > + } catch (IOException e) {
> > + // ignore
> > + }
> > }
> > }
> > }
> > Index: src/share/classes/com/sun/tck/cldc/javatest/TestBuilder.java
> > ===================================================================
> > --- src/share/classes/com/sun/tck/cldc/javatest/TestBuilder.java
> > (revision 272)
> > +++ src/share/classes/com/sun/tck/cldc/javatest/TestBuilder.java
> > (revision 273)
> > @@ -136,20 +136,20 @@
> > if (!fCheck.canRead()) {
> > throw new RuntimeException("TestBuilder: Cannot read
> > jar file: " + fCheck.getAbsolutePath());
> > }
> > - JarFile f = null;
> > + JarFile jarFile = null;
> > try {
> > - f = new JarFile(fname);
> > - Enumeration es = f.entries();
> > + jarFile = new JarFile(fname);
> > + Enumeration es = jarFile.entries();
> > while (es.hasMoreElements()) {
> > - JarEntry e = (JarEntry) es.nextElement();
> > - if (e.getName().startsWith("META-INF")) {
> > + JarEntry jarEntry = (JarEntry) es.nextElement();
> > + if (jarEntry.getName().startsWith("META-INF")) {
> > continue;
> > }
> > - data = new byte[(int) e.getSize()];
> > + data = new byte[(int) jarEntry.getSize()];
> >
> > InputStream is = null;
> > try {
> > - is = f.getInputStream(e);
> > + is = jarFile.getInputStream(jarEntry);
> > int chunk = 0;
> > int readed = 0;
> > while ((readed < data.length) && (chunk >= 0)) {
> > @@ -160,17 +160,21 @@
> > throw new RuntimeException("TestBuilder: " +
> > ioe.toString());
> > } finally {
> > if (is != null) {
> > - is.close();
> > + try {
> > + is.close();
> > + } catch (IOException e) {
> > + e.printStackTrace();
> > + }
> > }
> > }
> > - files.put(e.getName(), data);
> > + files.put(jarEntry.getName(), data);
> > }
> > } catch (IOException e) {
> > throw new RuntimeException("TestBuilder: " + e.toString());
> > } finally {
> > - if (f != null) {
> > + if (jarFile != null) {
> > try {
> > - f.close();
> > + jarFile.close();
> > } catch (Exception e) {
> > // Do nothing
> > }
> > Index: src/share/classes/com/sun/tck/midp/lib/OTASupportServer.java
> > ===================================================================
> > --- src/share/classes/com/sun/tck/midp/lib/OTASupportServer.java
> > (revision 272)
> > +++ src/share/classes/com/sun/tck/midp/lib/OTASupportServer.java
> > (revision 273)
> > @@ -748,12 +748,20 @@
> > sendDiagnostics(HTTP_BAD_REQUEST);
> > return;
> > } finally {
> > - try {
> > - dis.close();
> > - bais.close();
> > - } catch (IOException ioe) {
> > - // ignore, can't do much
> > + if (dis != null) {
> > + try {
> > + dis.close();
> > + } catch (IOException e) {
> > + // ignore
> > + }
> > }
> > + if (bais != null) {
> > + try {
> > + bais.close();
> > + } catch (IOException ioe) {
> > + // ignore
> > + }
> > + }
> > }
> >
> > sendDiagnostics(HTTP_OK);
> > Index:
> > src/share/classes/com/sun/tck/j2me/services/commService/CommToJavatestService.java
> > ===================================================================
> > ---
> > src/share/classes/com/sun/tck/j2me/services/commService/CommToJavatestService.java
> > (revision 272)
> > +++
> > src/share/classes/com/sun/tck/j2me/services/commService/CommToJavatestService.java
> > (revision 273)
> > @@ -167,7 +167,11 @@
> > }
> > } finally {
> > if (dos != null) {
> > - dos.close();
> > + try {
> > + dos.close();
> > + } catch (IOException e) {
> > + e.printStackTrace();
> > + }
> > }
> > }
> > }
> > @@ -221,7 +225,11 @@
> > }
> > } finally {
> > if (dos != null) {
> > - dos.close();
> > + try {
> > + dos.close();
> > + } catch (IOException e) {
> > + e.printStackTrace();
> > + }
> > }
> > }
> > }
> > @@ -270,7 +278,11 @@
> > dos.write(data);
> > } finally {
> > if (dos != null) {
> > - dos.close();
> > + try {
> > + dos.close();
> > + } catch (IOException e) {
> > + e.printStackTrace();
> > + }
> > }
> > }
> >
> > @@ -297,7 +309,11 @@
> > dos.writeInt(i);
> > } finally {
> > if (dos != null) {
> > - dos.close();
> > + try {
> > + dos.close();
> > + } catch (IOException e) {
> > + e.printStackTrace();
> > + }
> > }
> > }
> > byte[] resp = base.send(baos.toByteArray()); // send
> > request for the next portion
> > @@ -357,7 +373,11 @@
> > dis.readFully(respPacket);
> > } finally {
> > if (dis != null) {
> > - dis.close();
> > + try {
> > + dis.close();
> > + } catch (IOException e) {
> > + e.printStackTrace();
> > + }
> > }
> > }
> > }
> > Index:
> > src/share/classes/com/sun/tck/j2me/services/commService/clients/CldcHTTPCommClient.java
> > ===================================================================
> > ---
> > src/share/classes/com/sun/tck/j2me/services/commService/clients/CldcHTTPCommClient.java
> > (revision 272)
> > +++
> > src/share/classes/com/sun/tck/j2me/services/commService/clients/CldcHTTPCommClient.java
> > (revision 273)
> > @@ -114,19 +114,23 @@
> > if (is != null) {
> > try {
> > is.close();
> > - } catch (IOException e1) {
> > + } catch (IOException e) {
> > // Silently ignore
> > }
> > }
> > if (os != null) {
> > try {
> > os.close();
> > - } catch (IOException e1) {
> > + } catch (IOException e) {
> > // Silently ignore
> > }
> > }
> > if (hc != null) {
> > - hc.close();
> > + try {
> > + hc.close();
> > + } catch (IOException e) {
> > + e.printStackTrace();
> > + }
> > }
> > }
> > }
> > Index:
> > src/share/classes/com/sun/cldc/communication/midp/MIDHttpExecutionServer.java
> > ===================================================================
> > ---
> > src/share/classes/com/sun/cldc/communication/midp/MIDHttpExecutionServer.java
> > (revision 272)
> > +++
> > src/share/classes/com/sun/cldc/communication/midp/MIDHttpExecutionServer.java
> > (revision 273)
> > @@ -280,7 +280,11 @@
> > fos.flush();
> > } finally {
> > if (fos != null) {
> > - fos.close();
> > + try {
> > + fos.close();
> > + } catch (IOException e) {
> > + e.printStackTrace();
> > + }
> > }
> > }
> > }
> > @@ -530,13 +534,16 @@
> > File file = new File(serverRoot, path);
> > if (file.isFile() && file.canRead()) {
> > try {
> > - FileInputStream fis = new FileInputStream(file);
> > - DataInputStream fin = new DataInputStream(fis);
> > + DataInputStream fin = new DataInputStream(new
> > FileInputStream(file));
> > byte[] buf = new byte[(int) file.length()];
> > try {
> > fin.readFully(buf);
> > } finally {
> > - fin.close();
> > + try {
> > + fin.close();
> > + } catch (IOException e) {
> > + e.printStackTrace();
> > + }
> > }
> > setResponseProperty("Content-Length", "" +
> > file.length());
> > setResponseProperty("Content-Type",
> > Index: src/share/classes/com/sun/cldc/communication/midp/MIDHttpClient.java
> > ===================================================================
> > ---
> > src/share/classes/com/sun/cldc/communication/midp/MIDHttpClient.java
> > (revision 272)
> > +++
> > src/share/classes/com/sun/cldc/communication/midp/MIDHttpClient.java
> > (revision 273)
> > @@ -79,18 +79,21 @@
> > } catch (IOException e) {
> > throw new RuntimeException ("HttpClient: Unexpected
> > IOException while " +
> > "reading getNextTest - " + e);
> > - } finally {
> > - try {
> > - if (is != null) {
> > - is.close();
> > - }
> > - if (hc != null) {
> > - hc.close();
> > - }
> > - } catch (IOException e) {
> > - throw new RuntimeException ("HttpClient: Unexpected
> > IOException " +
> > - "while closing
> > communication - " + e);
> > + } finally {
> > + if (is != null) {
> > + try {
> > + is.close();
> > + } catch (IOException e) {
> > + e.printStackTrace();
> > + }
> > }
> > + if (hc != null) {
> > + try {
> > + hc.close();
> > + } catch (IOException e) {
> > + e.printStackTrace();
> > + }
> > + }
> > }
> > }
> >
> > @@ -101,7 +104,7 @@
> > try {
> > String dest = stream_url + "sendTestResult" + BundleID;
> > trace("Opening connection to " + dest);
> > - hc = (HttpConnection)Connector.open(dest,
> > Connector.READ_WRITE, false);
> > + hc = (HttpConnection) Connector.open(dest,
> > Connector.READ_WRITE, false);
> > hc.setRequestMethod("POST");
> > hc.setRequestProperty("Connection", "close");
> > trace("Posting data..");
> > @@ -118,17 +121,20 @@
> > throw new RuntimeException ("HttpClient: Unexpected
> > IOException while " +
> > "writing sendTestResult - " + e);
> > } finally {
> > - try {
> > - if (os != null) {
> > - os.close();
> > - }
> > - if (hc != null) {
> > - hc.close();
> > - }
> > - } catch (IOException e) {
> > - throw new RuntimeException ("HttpClient: Unexpected
> > IOException " +
> > - "while closing
> > communication - " + e);
> > + if (os != null) {
> > + try {
> > + os.close();
> > + } catch (IOException e) {
> > + e.printStackTrace();
> > + }
> > }
> > + if (hc != null) {
> > + try {
> > + hc.close();
> > + } catch (IOException e) {
> > + e.printStackTrace();
> > + }
> > + }
> > }
> > }
> >
> > Index:
> > src/share/classes/com/sun/cldc/communication/midp/DefaultContentHandler.java
> > ===================================================================
> > ---
> > src/share/classes/com/sun/cldc/communication/midp/DefaultContentHandler.java
> > (revision 272)
> > +++
> > src/share/classes/com/sun/cldc/communication/midp/DefaultContentHandler.java
> > (revision 273)
> > @@ -65,7 +65,11 @@
> > try {
> > fin.readFully(buf);
> > } finally {
> > - fin.close();
> > + try {
> > + fin.close();
> > + } catch (IOException e) {
> > + e.printStackTrace();
> > + }
> > }
> > server.setResponseProperty("Content-Length",
> > "" + jarFile.length());
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
> For additional commands, e-mail: meframework-help@cqme.dev.java.net
>

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Oleg Vazhnev

Vladimir Sizikov wrote:
> Hi,
>
> Small clarification below.
>
> On Mon, Jan 29, 2007 at 03:24:44AM -0800, Vladimir Sizikov wrote:
>
>> 2.
>> The bug also mentions the following code:
>>
>> try {
>> fin.readFully(buf);
>> } finally {
>> fin.close();
>> }
>>
>> I don't see anythirg wrong with it. IOEexception, thrown out of the
>> close() will propagate up the stack and be dealt with at the
>> appropriate level. There are no resource leaks in tihs case.
>>
>
> Well, to be absolutely correct, there is a small issue with
> possibility of the second IOE to hide the original one.
> But in our case I don't think that the problem is very real, and I
> don't think that it justifies the updates (and possibility to
> introduce regressions, etc).
>
> You see, in the case above, both exceptions are very similar and deal
> with the same stream and both mean that "something is wrong with the
> stream". So, there should no confusion and problems when investigating
> the exceptions.
> Also, exceptions out of close() are typically _very_ rare.
>

Yes, and it is exactly why such exceptions should be ignored.
I think e.printStackTrace() should be only in development version, but
not in released one.
Throwing an exception you say: "Oh, I can't continue execution, smth
very bad thing occurs!"
Is unable of release some systems resources really blocks program execution?
No.
We should continue do our work even if some file can't be closed or some
memory can't be released.
The really blocking issue is unnablabiliy of _reading_ smth, or
_creating_ smth, and we shouldn't hide really problem.
So I'm still believe my code is correct :-)

> I think it's better to leave this as is, and only fix the actual
> possible resource leaks.
>
> Thanks,
> --Vladimir
>
>
>> Not to mention the fact that in most cases the IOE will _never_ be
>> thrown due to fact that ByteArray streams are used and their close()
>> is a noop.
>>
>> In general, I think it's better to let the exception propagate then to
>> hide with:
>>
>> } catch (IOException e) {
>> e.printStackTrace();
>> }
>>
>> In fact, hiding the exception behind e.printStackTrace() is a know anti-pattern
>>
>> So, having said all of the above, let's focus on the actual cases when
>> resource leaks can happen.
>>
>> There are 3 such cases with possible resource leaks I see:
>>
>> unitTests/classes/com/sun/tck/j2me/services/messagingService/tests/MessageTests.java
>>
>> The change is OK, but I don't like the we completely hide the
>> IOE. Yes, it is not going to happen anyways, but if it's going to
>> happen on some broken implementation, we'll never know.
>>
>> Why not replace the two sequential close() calls to somithing like
>> this:
>>
>> try {
>> if (dos != null) {
>> dos.close();
>> }
>> } finally {
>> if (dis != null) {
>> dis.close();
>> }
>> }
>>
>> and let the IOE propagate up the stack, in that rare (if ever
>> possible) case when IOE is thrown.
>>
>> src/share/classes/com/sun/tck/midp/lib/OTASupportServer.java
>>
>> The 'dis' and 'bais' cannot be null, so there is need to check for
>> null here.
>>
>> Again, the subsequent invocations:
>> dis.close();
>> bais.close();
>>
>> can be replaced by much simple alternative:
>>
>> try {
>> dis.close();
>> finally {
>> bais.close();
>> }
>>
>> src/share/classes/com/sun/cldc/communication/midp/MIDHttpClient.java
>>
>> Similar to the first case, there is no need to change the original
>> code much (and definitely there is need to eliminante user-friendly
>> message.
>>
>> The rest of the changes mostly relate to hiding the IOException, and I
>> don't think we should do that.
>>
>> Thanks,
>> --Vladimir
>>
>>
>> On Sun, Jan 28, 2007 at 11:16:41PM +0300, Oleg Vazhnev wrote:
>>
>>> Vladimir
>>>
>>> Could you please review a bugfix for
>>> Issue #27: Possible resources leak in finally blocks
>>>
>>> Thanks
>>> --
>>> Oleg
>>>
>>> /opt/csw/bin/svn diff -r272:273
>>> https://cqme.dev.java.net/svn/cqme/branches/users/modelka/Issue27
>>> Index:
>>> unitTests/classes/com/sun/tck/j2me/services/messagingService/tests/MessageTests.java
>>> ===================================================================
>>> ---
>>> unitTests/classes/com/sun/tck/j2me/services/messagingService/tests/MessageTests.java
>>> (revision 272)
>>> +++
>>> unitTests/classes/com/sun/tck/j2me/services/messagingService/tests/MessageTests.java
>>> (revision 273)
>>> @@ -155,10 +155,18 @@
>>> assertEquals(msg_NullData, Message.read(dis));
>>> } finally {
>>> if (dos != null) {
>>> - dos.close();
>>> + try {
>>> + dos.close();
>>> + } catch (IOException e) {
>>> + // ignore
>>> + }
>>> }
>>> if (dis != null) {
>>> - dis.close();
>>> + try {
>>> + dis.close();
>>> + } catch (IOException e) {
>>> + // ignore
>>> + }
>>> }
>>> }
>>> }
>>> Index: src/share/classes/com/sun/tck/cldc/javatest/TestBuilder.java
>>> ===================================================================
>>> --- src/share/classes/com/sun/tck/cldc/javatest/TestBuilder.java
>>> (revision 272)
>>> +++ src/share/classes/com/sun/tck/cldc/javatest/TestBuilder.java
>>> (revision 273)
>>> @@ -136,20 +136,20 @@
>>> if (!fCheck.canRead()) {
>>> throw new RuntimeException("TestBuilder: Cannot read
>>> jar file: " + fCheck.getAbsolutePath());
>>> }
>>> - JarFile f = null;
>>> + JarFile jarFile = null;
>>> try {
>>> - f = new JarFile(fname);
>>> - Enumeration es = f.entries();
>>> + jarFile = new JarFile(fname);
>>> + Enumeration es = jarFile.entries();
>>> while (es.hasMoreElements()) {
>>> - JarEntry e = (JarEntry) es.nextElement();
>>> - if (e.getName().startsWith("META-INF")) {
>>> + JarEntry jarEntry = (JarEntry) es.nextElement();
>>> + if (jarEntry.getName().startsWith("META-INF")) {
>>> continue;
>>> }
>>> - data = new byte[(int) e.getSize()];
>>> + data = new byte[(int) jarEntry.getSize()];
>>>
>>> InputStream is = null;
>>> try {
>>> - is = f.getInputStream(e);
>>> + is = jarFile.getInputStream(jarEntry);
>>> int chunk = 0;
>>> int readed = 0;
>>> while ((readed < data.length) && (chunk >= 0)) {
>>> @@ -160,17 +160,21 @@
>>> throw new RuntimeException("TestBuilder: " +
>>> ioe.toString());
>>> } finally {
>>> if (is != null) {
>>> - is.close();
>>> + try {
>>> + is.close();
>>> + } catch (IOException e) {
>>> + e.printStackTrace();
>>> + }
>>> }
>>> }
>>> - files.put(e.getName(), data);
>>> + files.put(jarEntry.getName(), data);
>>> }
>>> } catch (IOException e) {
>>> throw new RuntimeException("TestBuilder: " + e.toString());
>>> } finally {
>>> - if (f != null) {
>>> + if (jarFile != null) {
>>> try {
>>> - f.close();
>>> + jarFile.close();
>>> } catch (Exception e) {
>>> // Do nothing
>>> }
>>> Index: src/share/classes/com/sun/tck/midp/lib/OTASupportServer.java
>>> ===================================================================
>>> --- src/share/classes/com/sun/tck/midp/lib/OTASupportServer.java
>>> (revision 272)
>>> +++ src/share/classes/com/sun/tck/midp/lib/OTASupportServer.java
>>> (revision 273)
>>> @@ -748,12 +748,20 @@
>>> sendDiagnostics(HTTP_BAD_REQUEST);
>>> return;
>>> } finally {
>>> - try {
>>> - dis.close();
>>> - bais.close();
>>> - } catch (IOException ioe) {
>>> - // ignore, can't do much
>>> + if (dis != null) {
>>> + try {
>>> + dis.close();
>>> + } catch (IOException e) {
>>> + // ignore
>>> + }
>>> }
>>> + if (bais != null) {
>>> + try {
>>> + bais.close();
>>> + } catch (IOException ioe) {
>>> + // ignore
>>> + }
>>> + }
>>> }
>>>
>>> sendDiagnostics(HTTP_OK);
>>> Index:
>>> src/share/classes/com/sun/tck/j2me/services/commService/CommToJavatestService.java
>>> ===================================================================
>>> ---
>>> src/share/classes/com/sun/tck/j2me/services/commService/CommToJavatestService.java
>>> (revision 272)
>>> +++
>>> src/share/classes/com/sun/tck/j2me/services/commService/CommToJavatestService.java
>>> (revision 273)
>>> @@ -167,7 +167,11 @@
>>> }
>>> } finally {
>>> if (dos != null) {
>>> - dos.close();
>>> + try {
>>> + dos.close();
>>> + } catch (IOException e) {
>>> + e.printStackTrace();
>>> + }
>>> }
>>> }
>>> }
>>> @@ -221,7 +225,11 @@
>>> }
>>> } finally {
>>> if (dos != null) {
>>> - dos.close();
>>> + try {
>>> + dos.close();
>>> + } catch (IOException e) {
>>> + e.printStackTrace();
>>> + }
>>> }
>>> }
>>> }
>>> @@ -270,7 +278,11 @@
>>> dos.write(data);
>>> } finally {
>>> if (dos != null) {
>>> - dos.close();
>>> + try {
>>> + dos.close();
>>> + } catch (IOException e) {
>>> + e.printStackTrace();
>>> + }
>>> }
>>> }
>>>
>>> @@ -297,7 +309,11 @@
>>> dos.writeInt(i);
>>> } finally {
>>> if (dos != null) {
>>> - dos.close();
>>> + try {
>>> + dos.close();
>>> + } catch (IOException e) {
>>> + e.printStackTrace();
>>> + }
>>> }
>>> }
>>> byte[] resp = base.send(baos.toByteArray()); // send
>>> request for the next portion
>>> @@ -357,7 +373,11 @@
>>> dis.readFully(respPacket);
>>> } finally {
>>> if (dis != null) {
>>> - dis.close();
>>> + try {
>>> + dis.close();
>>> + } catch (IOException e) {
>>> + e.printStackTrace();
>>> + }
>>> }
>>> }
>>> }
>>> Index:
>>> src/share/classes/com/sun/tck/j2me/services/commService/clients/CldcHTTPCommClient.java
>>> ===================================================================
>>> ---
>>> src/share/classes/com/sun/tck/j2me/services/commService/clients/CldcHTTPCommClient.java
>>> (revision 272)
>>> +++
>>> src/share/classes/com/sun/tck/j2me/services/commService/clients/CldcHTTPCommClient.java
>>> (revision 273)
>>> @@ -114,19 +114,23 @@
>>> if (is != null) {
>>> try {
>>> is.close();
>>> - } catch (IOException e1) {
>>> + } catch (IOException e) {
>>> // Silently ignore
>>> }
>>> }
>>> if (os != null) {
>>> try {
>>> os.close();
>>> - } catch (IOException e1) {
>>> + } catch (IOException e) {
>>> // Silently ignore
>>> }
>>> }
>>> if (hc != null) {
>>> - hc.close();
>>> + try {
>>> + hc.close();
>>> + } catch (IOException e) {
>>> + e.printStackTrace();
>>> + }
>>> }
>>> }
>>> }
>>> Index:
>>> src/share/classes/com/sun/cldc/communication/midp/MIDHttpExecutionServer.java
>>> ===================================================================
>>> ---
>>> src/share/classes/com/sun/cldc/communication/midp/MIDHttpExecutionServer.java
>>> (revision 272)
>>> +++
>>> src/share/classes/com/sun/cldc/communication/midp/MIDHttpExecutionServer.java
>>> (revision 273)
>>> @@ -280,7 +280,11 @@
>>> fos.flush();
>>> } finally {
>>> if (fos != null) {
>>> - fos.close();
>>> + try {
>>> + fos.close();
>>> + } catch (IOException e) {
>>> + e.printStackTrace();
>>> + }
>>> }
>>> }
>>> }
>>> @@ -530,13 +534,16 @@
>>> File file = new File(serverRoot, path);
>>> if (file.isFile() && file.canRead()) {
>>> try {
>>> - FileInputStream fis = new FileInputStream(file);
>>> - DataInputStream fin = new DataInputStream(fis);
>>> + DataInputStream fin = new DataInputStream(new
>>> FileInputStream(file));
>>> byte[] buf = new byte[(int) file.length()];
>>> try {
>>> fin.readFully(buf);
>>> } finally {
>>> - fin.close();
>>> + try {
>>> + fin.close();
>>> + } catch (IOException e) {
>>> + e.printStackTrace();
>>> + }
>>> }
>>> setResponseProperty("Content-Length", "" +
>>> file.length());
>>> setResponseProperty("Content-Type",
>>> Index: src/share/classes/com/sun/cldc/communication/midp/MIDHttpClient.java
>>> ===================================================================
>>> ---
>>> src/share/classes/com/sun/cldc/communication/midp/MIDHttpClient.java
>>> (revision 272)
>>> +++
>>> src/share/classes/com/sun/cldc/communication/midp/MIDHttpClient.java
>>> (revision 273)
>>> @@ -79,18 +79,21 @@
>>> } catch (IOException e) {
>>> throw new RuntimeException ("HttpClient: Unexpected
>>> IOException while " +
>>> "reading getNextTest - " + e);
>>> - } finally {
>>> - try {
>>> - if (is != null) {
>>> - is.close();
>>> - }
>>> - if (hc != null) {
>>> - hc.close();
>>> - }
>>> - } catch (IOException e) {
>>> - throw new RuntimeException ("HttpClient: Unexpected
>>> IOException " +
>>> - "while closing
>>> communication - " + e);
>>> + } finally {
>>> + if (is != null) {
>>> + try {
>>> + is.close();
>>> + } catch (IOException e) {
>>> + e.printStackTrace();
>>> + }
>>> }
>>> + if (hc != null) {
>>> + try {
>>> + hc.close();
>>> + } catch (IOException e) {
>>> + e.printStackTrace();
>>> + }
>>> + }
>>> }
>>> }
>>>
>>> @@ -101,7 +104,7 @@
>>> try {
>>> String dest = stream_url + "sendTestResult" + BundleID;
>>> trace("Opening connection to " + dest);
>>> - hc = (HttpConnection)Connector.open(dest,
>>> Connector.READ_WRITE, false);
>>> + hc = (HttpConnection) Connector.open(dest,
>>> Connector.READ_WRITE, false);
>>> hc.setRequestMethod("POST");
>>> hc.setRequestProperty("Connection", "close");
>>> trace("Posting data..");
>>> @@ -118,17 +121,20 @@
>>> throw new RuntimeException ("HttpClient: Unexpected
>>> IOException while " +
>>> "writing sendTestResult - " + e);
>>> } finally {
>>> - try {
>>> - if (os != null) {
>>> - os.close();
>>> - }
>>> - if (hc != null) {
>>> - hc.close();
>>> - }
>>> - } catch (IOException e) {
>>> - throw new RuntimeException ("HttpClient: Unexpected
>>> IOException " +
>>> - "while closing
>>> communication - " + e);
>>> + if (os != null) {
>>> + try {
>>> + os.close();
>>> + } catch (IOException e) {
>>> + e.printStackTrace();
>>> + }
>>> }
>>> + if (hc != null) {
>>> + try {
>>> + hc.close();
>>> + } catch (IOException e) {
>>> + e.printStackTrace();
>>> + }
>>> + }
>>> }
>>> }
>>>
>>> Index:
>>> src/share/classes/com/sun/cldc/communication/midp/DefaultContentHandler.java
>>> ===================================================================
>>> ---
>>> src/share/classes/com/sun/cldc/communication/midp/DefaultContentHandler.java
>>> (revision 272)
>>> +++
>>> src/share/classes/com/sun/cldc/communication/midp/DefaultContentHandler.java
>>> (revision 273)
>>> @@ -65,7 +65,11 @@
>>> try {
>>> fin.readFully(buf);
>>> } finally {
>>> - fin.close();
>>> + try {
>>> + fin.close();
>>> + } catch (IOException e) {
>>> + e.printStackTrace();
>>> + }
>>> }
>>> server.setResponseProperty("Content-Length",
>>> "" + jarFile.length());
>>>
>>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
>> For additional commands, e-mail: meframework-help@cqme.dev.java.net
>>
>>

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Vladimir Sizikov

Hi Oleg,

On Mon, Jan 29, 2007 at 03:41:57PM +0300, Oleg Vazhnev wrote:
> Yes, and it is exactly why such exceptions should be ignored.
> I think e.printStackTrace() should be only in development version, but
> not in released one.
> Throwing an exception you say: "Oh, I can't continue execution, smth
> very bad thing occurs!"
> Is unable of release some systems resources really blocks program execution?
> No.
> We should continue do our work even if some file can't be closed or some
> memory can't be released.
> The really blocking issue is unnablabiliy of _reading_ smth, or
> _creating_ smth, and we shouldn't hide really problem.
> So I'm still believe my code is correct :-)

I didn't say that your code was incorrect. I said that I probably
would not modify the sources to correct those issues. :) But since
you've already modified the code, and since we've already reviewed it
and discussed it to a length, why not commit indeed? ;)

So, please proceed with the commit.

Thanks,
--Vladimir

> >I think it's better to leave this as is, and only fix the actual
> >possible resource leaks.
> >
> >Thanks,
> > --Vladimir
> >
> >
> >>Not to mention the fact that in most cases the IOE will _never_ be
> >>thrown due to fact that ByteArray streams are used and their close()
> >>is a noop.
> >>
> >>In general, I think it's better to let the exception propagate then to
> >>hide with:
> >>
> >>} catch (IOException e) {
> >> e.printStackTrace();
> >>}
> >>
> >>In fact, hiding the exception behind e.printStackTrace() is a know
> >>anti-pattern
> >>
> >>So, having said all of the above, let's focus on the actual cases when
> >>resource leaks can happen.
> >>
> >>There are 3 such cases with possible resource leaks I see:
> >>
> >>unitTests/classes/com/sun/tck/j2me/services/messagingService/tests/MessageTests.java
> >>
> >>The change is OK, but I don't like the we completely hide the
> >>IOE. Yes, it is not going to happen anyways, but if it's going to
> >>happen on some broken implementation, we'll never know.
> >>
> >>Why not replace the two sequential close() calls to somithing like
> >>this:
> >>
> >>try {
> >> if (dos != null) {
> >> dos.close();
> >> }
> >>} finally {
> >> if (dis != null) {
> >> dis.close();
> >> }
> >>}
> >>
> >>and let the IOE propagate up the stack, in that rare (if ever
> >>possible) case when IOE is thrown.
> >>
> >>src/share/classes/com/sun/tck/midp/lib/OTASupportServer.java
> >>
> >>The 'dis' and 'bais' cannot be null, so there is need to check for
> >>null here.
> >>
> >>Again, the subsequent invocations:
> >>dis.close();
> >>bais.close();
> >>
> >>can be replaced by much simple alternative:
> >>
> >>try {
> >> dis.close();
> >>finally {
> >> bais.close();
> >>}
> >>
> >>src/share/classes/com/sun/cldc/communication/midp/MIDHttpClient.java
> >>
> >>Similar to the first case, there is no need to change the original
> >>code much (and definitely there is need to eliminante user-friendly
> >>message.
> >>
> >>The rest of the changes mostly relate to hiding the IOException, and I
> >>don't think we should do that.
> >>
> >>Thanks,
> >> --Vladimir
> >>
> >>
> >>On Sun, Jan 28, 2007 at 11:16:41PM +0300, Oleg Vazhnev wrote:
> >>
> >>>Vladimir
> >>>
> >>>Could you please review a bugfix for
> >>>Issue #27: Possible resources leak in finally blocks
> >>>
> >>>Thanks
> >>>--
> >>>Oleg
> >>>
> >>>/opt/csw/bin/svn diff -r272:273
> >>>https://cqme.dev.java.net/svn/cqme/branches/users/modelka/Issue27
> >>>Index:
> >>>unitTests/classes/com/sun/tck/j2me/services/messagingService/tests/MessageTests.java
> >>>===================================================================
> >>>---
> >>>unitTests/classes/com/sun/tck/j2me/services/messagingService/tests/MessageTests.java
> >>>(revision 272)
> >>>+++
> >>>unitTests/classes/com/sun/tck/j2me/services/messagingService/tests/MessageTests.java
> >>>(revision 273)
> >>>@@ -155,10 +155,18 @@
> >>> assertEquals(msg_NullData, Message.read(dis));
> >>> } finally {
> >>> if (dos != null) {
> >>>- dos.close();
> >>>+ try {
> >>>+ dos.close();
> >>>+ } catch (IOException e) {
> >>>+ // ignore
> >>>+ }
> >>> }
> >>> if (dis != null) {
> >>>- dis.close();
> >>>+ try {
> >>>+ dis.close();
> >>>+ } catch (IOException e) {
> >>>+ // ignore
> >>>+ }
> >>> }
> >>> }
> >>> }
> >>>Index: src/share/classes/com/sun/tck/cldc/javatest/TestBuilder.java
> >>>===================================================================
> >>>--- src/share/classes/com/sun/tck/cldc/javatest/TestBuilder.java
> >>>(revision 272)
> >>>+++ src/share/classes/com/sun/tck/cldc/javatest/TestBuilder.java
> >>>(revision 273)
> >>>@@ -136,20 +136,20 @@
> >>> if (!fCheck.canRead()) {
> >>> throw new RuntimeException("TestBuilder: Cannot read
> >>>jar file: " + fCheck.getAbsolutePath());
> >>> }
> >>>- JarFile f = null;
> >>>+ JarFile jarFile = null;
> >>> try {
> >>>- f = new JarFile(fname);
> >>>- Enumeration es = f.entries();
> >>>+ jarFile = new JarFile(fname);
> >>>+ Enumeration es = jarFile.entries();
> >>> while (es.hasMoreElements()) {
> >>>- JarEntry e = (JarEntry) es.nextElement();
> >>>- if (e.getName().startsWith("META-INF")) {
> >>>+ JarEntry jarEntry = (JarEntry) es.nextElement();
> >>>+ if (jarEntry.getName().startsWith("META-INF")) {
> >>> continue;
> >>> }
> >>>- data = new byte[(int) e.getSize()];
> >>>+ data = new byte[(int) jarEntry.getSize()];
> >>>
> >>> InputStream is = null;
> >>> try {
> >>>- is = f.getInputStream(e);
> >>>+ is = jarFile.getInputStream(jarEntry);
> >>> int chunk = 0;
> >>> int readed = 0;
> >>> while ((readed < data.length) && (chunk >= 0)) {
> >>>@@ -160,17 +160,21 @@
> >>> throw new RuntimeException("TestBuilder: " +
> >>>ioe.toString());
> >>> } finally {
> >>> if (is != null) {
> >>>- is.close();
> >>>+ try {
> >>>+ is.close();
> >>>+ } catch (IOException e) {
> >>>+ e.printStackTrace();
> >>>+ }
> >>> }
> >>> }
> >>>- files.put(e.getName(), data);
> >>>+ files.put(jarEntry.getName(), data);
> >>> }
> >>> } catch (IOException e) {
> >>> throw new RuntimeException("TestBuilder: " +
> >>> e.toString());
> >>> } finally {
> >>>- if (f != null) {
> >>>+ if (jarFile != null) {
> >>> try {
> >>>- f.close();
> >>>+ jarFile.close();
> >>> } catch (Exception e) {
> >>> // Do nothing
> >>> }
> >>>Index: src/share/classes/com/sun/tck/midp/lib/OTASupportServer.java
> >>>===================================================================
> >>>--- src/share/classes/com/sun/tck/midp/lib/OTASupportServer.java
> >>>(revision 272)
> >>>+++ src/share/classes/com/sun/tck/midp/lib/OTASupportServer.java
> >>>(revision 273)
> >>>@@ -748,12 +748,20 @@
> >>> sendDiagnostics(HTTP_BAD_REQUEST);
> >>> return;
> >>> } finally {
> >>>- try {
> >>>- dis.close();
> >>>- bais.close();
> >>>- } catch (IOException ioe) {
> >>>- // ignore, can't do much
> >>>+ if (dis != null) {
> >>>+ try {
> >>>+ dis.close();
> >>>+ } catch (IOException e) {
> >>>+ // ignore
> >>>+ }
> >>> }
> >>>+ if (bais != null) {
> >>>+ try {
> >>>+ bais.close();
> >>>+ } catch (IOException ioe) {
> >>>+ // ignore
> >>>+ }
> >>>+ }
> >>> }
> >>>
> >>> sendDiagnostics(HTTP_OK);
> >>>Index:
> >>>src/share/classes/com/sun/tck/j2me/services/commService/CommToJavatestService.java
> >>>===================================================================
> >>>---
> >>>src/share/classes/com/sun/tck/j2me/services/commService/CommToJavatestService.java
> >>>(revision 272)
> >>>+++
> >>>src/share/classes/com/sun/tck/j2me/services/commService/CommToJavatestService.java
> >>>(revision 273)
> >>>@@ -167,7 +167,11 @@
> >>> }
> >>> } finally {
> >>> if (dos != null) {
> >>>- dos.close();
> >>>+ try {
> >>>+ dos.close();
> >>>+ } catch (IOException e) {
> >>>+ e.printStackTrace();
> >>>+ }
> >>> }
> >>> }
> >>> }
> >>>@@ -221,7 +225,11 @@
> >>> }
> >>> } finally {
> >>> if (dos != null) {
> >>>- dos.close();
> >>>+ try {
> >>>+ dos.close();
> >>>+ } catch (IOException e) {
> >>>+ e.printStackTrace();
> >>>+ }
> >>> }
> >>> }
> >>> }
> >>>@@ -270,7 +278,11 @@
> >>> dos.write(data);
> >>> } finally {
> >>> if (dos != null) {
> >>>- dos.close();
> >>>+ try {
> >>>+ dos.close();
> >>>+ } catch (IOException e) {
> >>>+ e.printStackTrace();
> >>>+ }
> >>> }
> >>> }
> >>>
> >>>@@ -297,7 +309,11 @@
> >>> dos.writeInt(i);
> >>> } finally {
> >>> if (dos != null) {
> >>>- dos.close();
> >>>+ try {
> >>>+ dos.close();
> >>>+ } catch (IOException e) {
> >>>+ e.printStackTrace();
> >>>+ }
> >>> }
> >>> }
> >>> byte[] resp = base.send(baos.toByteArray()); // send
> >>>request for the next portion
> >>>@@ -357,7 +373,11 @@
> >>> dis.readFully(respPacket);
> >>> } finally {
> >>> if (dis != null) {
> >>>- dis.close();
> >>>+ try {
> >>>+ dis.close();
> >>>+ } catch (IOException e) {
> >>>+ e.printStackTrace();
> >>>+ }
> >>> }
> >>> }
> >>> }
> >>>Index:
> >>>src/share/classes/com/sun/tck/j2me/services/commService/clients/CldcHTTPCommClient.java
> >>>===================================================================
> >>>---
> >>>src/share/classes/com/sun/tck/j2me/services/commService/clients/CldcHTTPCommClient.java
> >>>(revision 272)
> >>>+++
> >>>src/share/classes/com/sun/tck/j2me/services/commService/clients/CldcHTTPCommClient.java
> >>>(revision 273)
> >>>@@ -114,19 +114,23 @@
> >>> if (is != null) {
> >>> try {
> >>> is.close();
> >>>- } catch (IOException e1) {
> >>>+ } catch (IOException e) {
> >>> // Silently ignore
> >>> }
> >>> }
> >>> if (os != null) {
> >>> try {
> >>> os.close();
> >>>- } catch (IOException e1) {
> >>>+ } catch (IOException e) {
> >>> // Silently ignore
> >>> }
> >>> }
> >>> if (hc != null) {
> >>>- hc.close();
> >>>+ try {
> >>>+ hc.close();
> >>>+ } catch (IOException e) {
> >>>+ e.printStackTrace();
> >>>+ }
> >>> }
> >>> }
> >>> }
> >>>Index:
> >>>src/share/classes/com/sun/cldc/communication/midp/MIDHttpExecutionServer.java
> >>>===================================================================
> >>>---
> >>>src/share/classes/com/sun/cldc/communication/midp/MIDHttpExecutionServer.java
> >>>(revision 272)
> >>>+++
> >>>src/share/classes/com/sun/cldc/communication/midp/MIDHttpExecutionServer.java
> >>>(revision 273)
> >>>@@ -280,7 +280,11 @@
> >>> fos.flush();
> >>> } finally {
> >>> if (fos != null) {
> >>>- fos.close();
> >>>+ try {
> >>>+ fos.close();
> >>>+ } catch (IOException e) {
> >>>+ e.printStackTrace();
> >>>+ }
> >>> }
> >>> }
> >>> }
> >>>@@ -530,13 +534,16 @@
> >>> File file = new File(serverRoot, path);
> >>> if (file.isFile() && file.canRead()) {
> >>> try {
> >>>- FileInputStream fis = new FileInputStream(file);
> >>>- DataInputStream fin = new DataInputStream(fis);
> >>>+ DataInputStream fin = new DataInputStream(new
> >>>FileInputStream(file));
> >>> byte[] buf = new byte[(int) file.length()];
> >>> try {
> >>> fin.readFully(buf);
> >>> } finally {
> >>>- fin.close();
> >>>+ try {
> >>>+ fin.close();
> >>>+ } catch (IOException e) {
> >>>+ e.printStackTrace();
> >>>+ }
> >>> }
> >>> setResponseProperty("Content-Length", "" +
> >>>file.length());
> >>> setResponseProperty("Content-Type",
> >>>Index:
> >>>src/share/classes/com/sun/cldc/communication/midp/MIDHttpClient.java
> >>>===================================================================
> >>>---
> >>>src/share/classes/com/sun/cldc/communication/midp/MIDHttpClient.java
> >>>(revision 272)
> >>>+++
> >>>src/share/classes/com/sun/cldc/communication/midp/MIDHttpClient.java
> >>>(revision 273)
> >>>@@ -79,18 +79,21 @@
> >>> } catch (IOException e) {
> >>> throw new RuntimeException ("HttpClient: Unexpected
> >>>IOException while " +
> >>> "reading getNextTest - " + e);
> >>>- } finally {
> >>>- try {
> >>>- if (is != null) {
> >>>- is.close();
> >>>- }
> >>>- if (hc != null) {
> >>>- hc.close();
> >>>- }
> >>>- } catch (IOException e) {
> >>>- throw new RuntimeException ("HttpClient: Unexpected
> >>>IOException " +
> >>>- "while closing
> >>>communication - " + e);
> >>>+ } finally {
> >>>+ if (is != null) {
> >>>+ try {
> >>>+ is.close();
> >>>+ } catch (IOException e) {
> >>>+ e.printStackTrace();
> >>>+ }
> >>> }
> >>>+ if (hc != null) {
> >>>+ try {
> >>>+ hc.close();
> >>>+ } catch (IOException e) {
> >>>+ e.printStackTrace();
> >>>+ }
> >>>+ }
> >>> }
> >>> }
> >>>
> >>>@@ -101,7 +104,7 @@
> >>> try {
> >>> String dest = stream_url + "sendTestResult" + BundleID;
> >>> trace("Opening connection to " + dest);
> >>>- hc = (HttpConnection)Connector.open(dest,
> >>>Connector.READ_WRITE, false);
> >>>+ hc = (HttpConnection) Connector.open(dest,
> >>>Connector.READ_WRITE, false);
> >>> hc.setRequestMethod("POST");
> >>> hc.setRequestProperty("Connection", "close");
> >>> trace("Posting data..");
> >>>@@ -118,17 +121,20 @@
> >>> throw new RuntimeException ("HttpClient: Unexpected
> >>>IOException while " +
> >>> "writing sendTestResult - " + e);
> >>> } finally {
> >>>- try {
> >>>- if (os != null) {
> >>>- os.close();
> >>>- }
> >>>- if (hc != null) {
> >>>- hc.close();
> >>>- }
> >>>- } catch (IOException e) {
> >>>- throw new RuntimeException ("HttpClient: Unexpected
> >>>IOException " +
> >>>- "while closing
> >>>communication - " + e);
> >>>+ if (os != null) {
> >>>+ try {
> >>>+ os.close();
> >>>+ } catch (IOException e) {
> >>>+ e.printStackTrace();
> >>>+ }
> >>> }
> >>>+ if (hc != null) {
> >>>+ try {
> >>>+ hc.close();
> >>>+ } catch (IOException e) {
> >>>+ e.printStackTrace();
> >>>+ }
> >>>+ }
> >>> }
> >>> }
> >>>
> >>>Index:
> >>>src/share/classes/com/sun/cldc/communication/midp/DefaultContentHandler.java
> >>>===================================================================
> >>>---
> >>>src/share/classes/com/sun/cldc/communication/midp/DefaultContentHandler.java
> >>>(revision 272)
> >>>+++
> >>>src/share/classes/com/sun/cldc/communication/midp/DefaultContentHandler.java
> >>>(revision 273)
> >>>@@ -65,7 +65,11 @@
> >>> try {
> >>> fin.readFully(buf);
> >>> } finally {
> >>>- fin.close();
> >>>+ try {
> >>>+ fin.close();
> >>>+ } catch (IOException e) {
> >>>+ e.printStackTrace();
> >>>+ }
> >>> }
> >>> server.setResponseProperty("Content-Length",
> >>> "" + jarFile.length());
> >>>
> >>>
> >>---------------------------------------------------------------------
> >>To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
> >>For additional commands, e-mail: meframework-help@cqme.dev.java.net
> >>
> >>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net