Hi all,
I've posted a webrev for this at:
http://cr.openjdk.java.net/~michaelm/7085981/webrev.1/
After checking Socket and ServerSocket, I believe they are not actually
affected by this issue. The IllegalArgumentExceptions are thrown before the
impl is created in both cases.
Thanks
Michael.
On 01/09/11 18:33, Michael McMahon wrote:
Right. That would work assuming createImpl() doesn't leave the socket
open
if it throws an exception, which seems to be the case.
On 01/09/11 18:05, Salter, Thomas A wrote:
Maybe I posted a bad patch, but my intent was to do the try-finally
after checking for a non-null bind address.
public DatagramSocket(SocketAddress bindaddr) throws
SocketException {
// create a datagram socket.
createImpl();
if (bindaddr != null) {
try {
bind(bindaddr);
} finally {
if( !isBound() )
close();
}
}
}
On a related note, I've also noticed that the Socket and ServerSocket
constructors can throw without closing the implementation, but this
only happens with IllegalArgumentException or other
RuntimeExceptions. I'm not sure what your policy is on cleaning up
after runtime exceptions.
The SocketImpl finalizer does eventually clean up. But, I agree we
shouldn't rely on that
especially in cases like the IAE explicitly thrown in the constructor.
We'll include those
cases in the fix for this bug as well.
Michael.
-----Original Message-----
From: Michael McMahon [mailto:michael.x.mcma...@oracle.com]
Sent: Thursday, September 01, 2011 12:47 PM
To: Salter, Thomas A
Cc: net-dev@openjdk.java.net
Subject: Re: Datagram socket leak
Thomas,
Thanks for pointing this out. We overlooked this in the recent change in
this
area. One thing though, in the second change to DatagramSocket we can't
just check for isBound() since the socket might legitimately be unbound
(bindaddr is null). All I can think is that we catch the exception and
re throw
it, after closing, rather than use a finally() in that case. I have
created a bug report (7085981)
to track this. I'll post a webrev for it soon.
- Michael.
On 29/08/11 20:01, Salter, Thomas A wrote:
Here's what I changed. I'm working with the fcs source bundle for
b147, 27_jun_2011, so I may not have the latest source base.
Left base folder: new
Right base folder: b147
File: src\share\classes\java\net\DatagramSocket.java
186,189d185
< finally {
< if( !isBound() )
< close();
< }
234d229
< try {
236,239d230
< } finally {
< if( !isBound() )
< close();
< }
File: src\share\classes\java\net\MulticastSocket.java
165d164
< try {
167,170d165
< } finally {
< if( !isBound() )
< close();
< }
-----Original Message-----
From: Chris Hegarty [mailto:chris.hega...@oracle.com]
Sent: Monday, August 29, 2011 2:33 PM
To: Salter, Thomas A
Cc: net-dev@openjdk.java.net
Subject: Re: Datagram socket leak
Ah ok. I finally get it.
In which case I think you original changes should be fine. Do you want
to make similar changes to MulticastSocket and post the diffs?
Also, I think a testcase would be useful here. I know it's not strictly
specified that the socket should be closed if the constructor throws,
but it does seem desirable.
-Chris.
On 08/29/11 07:14 PM, Salter, Thomas A wrote:
I believe you're referring to the close() in the catch clause
following the call to getImpl().bind. The problem I encountered
was when the Datagram.bind threw an exception before it got that
far. In my case, the checkListen was throwing a SecurityException,
but any of the earlier throws would cause the same problem. The
SecurityException wouldn't have been caught by the catch addressed
by the CR in any case. We encountered this while running the TCK.
One of its tests tries to create lots of sockets, all of them
getting security violations until we hit a limit on the number of
open sockets.
public synchronized void bind(SocketAddress addr) throws
SocketException {
if (isClosed())
throw new SocketException("Socket is closed");
if (isBound())
throw new SocketException("already bound");
if (addr == null)
addr = new InetSocketAddress(0);
if (!(addr instanceof InetSocketAddress))
throw new IllegalArgumentException("Unsupported
address type!");
InetSocketAddress epoint = (InetSocketAddress) addr;
if (epoint.isUnresolved())
throw new SocketException("Unresolved address");
InetAddress iaddr = epoint.getAddress();
int port = epoint.getPort();
checkAddress(iaddr, "bind");
SecurityManager sec = System.getSecurityManager();
if (sec != null) {
sec.checkListen(port);<<<<<<<<<<<<<<<<<<<<<<<<<<
This throws a SecurityException
}
try {
getImpl().bind(port, iaddr);
} catch (SocketException e) {
getImpl().close();
throw e;
}
bound = true;
}
Tom.
-----Original Message-----
From: Chris Hegarty [mailto:chris.hega...@oracle.com]
Sent: Monday, August 29, 2011 1:56 PM
To: Salter, Thomas A
Cc: net-dev@openjdk.java.net
Subject: Re: Datagram socket leak
[take two!]
Tom,
This specific area of code was changed recently due to CR 7035556 [1],
changeset [2], and this issue was discussed during the code review
[3].
Essentially, bind() already closes the impl internally before throwing
the exception. Does this resolve the issue for you? Or do you still
see
potential to leak?
-Chris
[1] http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7035556
[2] http://hg.openjdk.java.net/jdk8/tl/jdk/rev/07a12583d4ea
[3]
http://mail.openjdk.java.net/pipermail/net-dev/2011-July/003318.html
On 08/29/11 03:27 PM, Salter, Thomas A wrote:
There appears to be a socket leak in both DatagramSocket and
MulticastSocket constructors. Both classes have constructors that
create
a socket and then attempt to bind. The bind can fail with a
variety of
exceptions none of which are caught by the constructor. Thus, the
actual
system socket that was allocated by impl.create() is never closed.
My fix was to wrap a try-finally around the bind call and call
close()
if isBound is false.
public DatagramSocket(SocketAddress bindaddr) throws
SocketException {
// create a datagram socket.
createImpl();
if (bindaddr != null) {
try {
bind(bindaddr);
} finally {
if( !isBound() )
close();
}
}
}
Tom Salter
Unisys Corporation
