Re: Code Review Request: 7035556 DatagramSocket.java:183: warning: unreachable catch clause
There seems to be only a couple of places where IOException
(specifically) is thrown
in the networking code and it can't occur in this code path as far as I
can see.
Perhaps that wasn't always the case, which could explain why the unusual
construction exists.
So, the simplification suggested below seems reasonable.
- Michael.
On 21/07/11 23:54, Brad Wetmore wrote:
Kurchi,
Please have an additional reviewer for the networking portion of this,
but in doing a quick look, I believe the point of the two catches is
to make sure that any IOException that isn't already a SocketException
(SocketException is a subclass of IOException) is rebranded to
SocketException.
If these internal methods are guaranteed to only throw SocketExeptions
(and it looks like it), then you can simplify this code to:
public DatagramSocket() throws SocketException {
createImpl();
bind(new InetSocketAddress(0));
}
bind throws SocketException, InetSocketAddress(int) only throws a IAE
(a RuntimeException).
Brad
On 7/21/2011 3:05 PM, Kurchi Hazra wrote:
Hi,
Due to a recent update in javac to issue a warning on detecting
unreachable code, the following warning started showing up in the jdk
networking code:
../../../src/share/classes/java/net/DatagramSocket.java:183: warning:
unreachable catch clause.
This fix aims at removing this warning by removing the IOException. On
inspection, it was found that currently, the native code does not throw
any IOException.
The fix involves updates in:
jdk/src/share/classes/java/net/DatagramSocket.java
Webrev: http://cr.openjdk.java.net/~chegar/7035556/webrev.00/
Thanks,
-Kurchi
Re: Code Review Request: 7035556 DatagramSocket.java:183: warning: unreachable catch clause
On 7/22/2011 3:45 AM, Alan Bateman wrote: Kurchi Hazra wrote: Hi, Due to a recent update in javac to issue a warning on detecting unreachable code, the following warning started showing up in the jdk networking code: ../../../src/share/classes/java/net/DatagramSocket.java:183: warning: unreachable catch clause. This fix aims at removing this warning by removing the IOException. On inspection, it was found that currently, the native code does not throw any IOException. The fix involves updates in: jdk/src/share/classes/java/net/DatagramSocket.java Webrev: http://cr.openjdk.java.net/~chegar/7035556/webrev.00/ Thanks, -Kurchi Kurchi - one suggestion is to close the UDP socket in the event that the bind fails. That would be nicer than leaving it to the impl's finalizer. Ah yes, makes sense. thanks for catching this Alan. -Chris. -Alan.
Re: Code Review Request: 7035556 DatagramSocket.java:183: warning: unreachable catch clause
On 22/07/11 13:51, Chris Hegarty wrote: On 7/22/2011 3:45 AM, Alan Bateman wrote: Kurchi Hazra wrote: Hi, Due to a recent update in javac to issue a warning on detecting unreachable code, the following warning started showing up in the jdk networking code: ../../../src/share/classes/java/net/DatagramSocket.java:183: warning: unreachable catch clause. This fix aims at removing this warning by removing the IOException. On inspection, it was found that currently, the native code does not throw any IOException. The fix involves updates in: jdk/src/share/classes/java/net/DatagramSocket.java Webrev: http://cr.openjdk.java.net/~chegar/7035556/webrev.00/ Thanks, -Kurchi Kurchi - one suggestion is to close the UDP socket in the event that the bind fails. That would be nicer than leaving it to the impl's finalizer. Ah yes, makes sense. thanks for catching this Alan. But, bind() already closes the impl internally before throwing the exception. - Michael
Re: Code Review Request: 7035556 DatagramSocket.java:183: warning: unreachable catch clause
On 7/22/2011 2:41 PM, Michael McMahon wrote: On 22/07/11 13:51, Chris Hegarty wrote: On 7/22/2011 3:45 AM, Alan Bateman wrote: Kurchi Hazra wrote: Hi, Due to a recent update in javac to issue a warning on detecting unreachable code, the following warning started showing up in the jdk networking code: ../../../src/share/classes/java/net/DatagramSocket.java:183: warning: unreachable catch clause. This fix aims at removing this warning by removing the IOException. On inspection, it was found that currently, the native code does not throw any IOException. The fix involves updates in: jdk/src/share/classes/java/net/DatagramSocket.java Webrev: http://cr.openjdk.java.net/~chegar/7035556/webrev.00/ Thanks, -Kurchi Kurchi - one suggestion is to close the UDP socket in the event that the bind fails. That would be nicer than leaving it to the impl's finalizer. Ah yes, makes sense. thanks for catching this Alan. But, bind() already closes the impl internally before throwing the exception. Oh, I didn't notice this. Great, then your original comment stands ( simply remove all catches ). -Chris. - Michael
Re: Code Review Request: 7035556 DatagramSocket.java:183: warning: unreachable catch clause
Michael McMahon wrote: But, bind() already closes the impl internally before throwing the exception. I was wondering about that and whether this is a bug. Suppose someone creates an unbound DatagramSocket and then attempts to bind it to a port. If the bind fails (say port already in use) then it may be surprising that they can't retry with a different port. Should this be specified? I see there are cases such as the security exception where it doesn't the close the impl. -Alan.
Re: Code Review Request: 7035556 DatagramSocket.java:183: warning: unreachable catch clause
On 22/07/11 14:55, Alan Bateman wrote: Michael McMahon wrote: But, bind() already closes the impl internally before throwing the exception. I was wondering about that and whether this is a bug. Suppose someone creates an unbound DatagramSocket and then attempts to bind it to a port. If the bind fails (say port already in use) then it may be surprising that they can't retry with a different port. Should this be specified? I see there are cases such as the security exception where it doesn't the close the impl. -Alan. It doesn't seem to be specified either way, though it does seem to be inconsistent. I'd be wary about changing the behaviour though unless there was a strong justification (more than a compiler warning :) ) - Michael.
Re: Code Review Request: 7035556 DatagramSocket.java:183: warning: unreachable catch clause
Hi,
I changed the implementation according to Brad's comments. I am
reposting the output of hg diff
src/share/classes/java/net/DatagramSocket.java since I don't have an
openjdk account:
bash-3.00$ hg diff src/share/classes/java/net/DatagramSocket.java
diff --git a/src/share/classes/java/net/DatagramSocket.java
b/src/share/classes/java/net/DatagramSocket.java
--- a/src/share/classes/java/net/DatagramSocket.java
+++ b/src/share/classes/java/net/DatagramSocket.java
@@ -176,13 +176,7 @@ class DatagramSocket implements java.io.
public DatagramSocket() throws SocketException {
// create a datagram socket.
createImpl();
-try {
-bind(new InetSocketAddress(0));
-} catch (SocketException se) {
-throw se;
-} catch(IOException e) {
-throw new SocketException(e.getMessage());
-}
+bind(new InetSocketAddress(0));
}
Thanks,
Kurchi
On 7/22/2011 7:25 AM, Michael McMahon wrote:
On 22/07/11 14:55, Alan Bateman wrote:
Michael McMahon wrote:
But, bind() already closes the impl internally before throwing the
exception.
I was wondering about that and whether this is a bug. Suppose someone
creates an unbound DatagramSocket and then attempts to bind it to a
port. If the bind fails (say port already in use) then it may be
surprising that they can't retry with a different port. Should this
be specified? I see there are cases such as the security exception
where it doesn't the close the impl.
-Alan.
It doesn't seem to be specified either way, though it does seem to be
inconsistent.
I'd be wary about changing the behaviour though
unless there was a strong justification (more than a compiler warning :) )
- Michael.
hg: jdk8/tl/corba: 7069993: Adjust make/jprt.properties file for jdk8
Changeset: 949fb60ca830 Author:ohair Date: 2011-07-22 17:34 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/corba/rev/949fb60ca830 7069993: Adjust make/jprt.properties file for jdk8 Reviewed-by: katleman ! make/jprt.properties
hg: jdk8/tl/jaxws: 7069993: Adjust make/jprt.properties file for jdk8
Changeset: 64df57a1edec Author:ohair Date: 2011-07-22 17:35 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jaxws/rev/64df57a1edec 7069993: Adjust make/jprt.properties file for jdk8 Reviewed-by: katleman ! make/jprt.properties
hg: jdk8/tl/jdk: 2 new changesets
Changeset: 0ec4b6498a69 Author:ohair Date: 2011-07-22 17:35 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/0ec4b6498a69 7069993: Adjust make/jprt.properties file for jdk8 Reviewed-by: katleman ! make/jprt.properties Changeset: a499fdfbe723 Author:ohair Date: 2011-07-22 21:31 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/a499fdfbe723 Merge
hg: jdk8/tl/jaxp: 7069993: Adjust make/jprt.properties file for jdk8
Changeset: 4f0fcb812767 Author:ohair Date: 2011-07-22 17:34 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jaxp/rev/4f0fcb812767 7069993: Adjust make/jprt.properties file for jdk8 Reviewed-by: katleman ! make/jprt.properties
hg: jdk8/tl: 2 new changesets
Changeset: fd8615098a54 Author:ohair Date: 2011-07-22 17:35 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/rev/fd8615098a54 7069993: Adjust make/jprt.properties file for jdk8 Reviewed-by: katleman ! make/jprt.properties Changeset: f42e3d9394b4 Author:ohair Date: 2011-07-22 21:31 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/rev/f42e3d9394b4 Merge
hg: jdk8/tl/langtools: 2 new changesets
Changeset: 2d3096441387 Author:ohair Date: 2011-07-22 17:35 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/2d3096441387 7069993: Adjust make/jprt.properties file for jdk8 Reviewed-by: katleman ! make/jprt.properties Changeset: 36f31b87b0ab Author:ohair Date: 2011-07-22 21:31 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/36f31b87b0ab Merge
