You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@harmony.apache.org by nd...@apache.org on 2009/10/11 05:13:44 UTC
svn commit: r824006 - in
/harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net:
DatagramPacket.java DatagramSocket.java
Author: ndbeyer
Date: Sun Oct 11 03:13:39 2009
New Revision: 824006
URL: http://svn.apache.org/viewvc?rev=824006&view=rev
Log:
add some package methods to provide consistent synchronization of access to instance fields
Modified:
harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net/DatagramPacket.java
harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net/DatagramSocket.java
Modified: harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net/DatagramPacket.java
URL: http://svn.apache.org/viewvc/harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net/DatagramPacket.java?rev=824006&r1=824005&r2=824006&view=diff
==============================================================================
--- harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net/DatagramPacket.java (original)
+++ harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net/DatagramPacket.java Sun Oct 11 03:13:39 2009
@@ -215,6 +215,15 @@
}
/**
+ * Gets the current capacity value.
+ *
+ * @return the current capacity value
+ */
+ synchronized int getCapacity() {
+ return capacity;
+ }
+
+ /**
* Sets the length of the datagram packet. This length plus the offset must
* be lesser than or equal to the buffer size.
*
@@ -230,6 +239,19 @@
}
/**
+ * An alternative to {@link #setLength(int)}, that doesn't reset the {@link #capacity}
+ * field.
+ *
+ * @param len the length of this datagram packet
+ */
+ synchronized void setLengthOnly(int len) {
+ if (0 > len || offset + len > data.length) {
+ throw new IllegalArgumentException(Msg.getString("K002f")); //$NON-NLS-1$
+ }
+ length = len;
+ }
+
+ /**
* Sets the port number of the target host of this datagram packet.
*
* @param aPort
Modified: harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net/DatagramSocket.java
URL: http://svn.apache.org/viewvc/harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net/DatagramSocket.java?rev=824006&r1=824005&r2=824006&view=diff
==============================================================================
--- harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net/DatagramSocket.java (original)
+++ harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net/DatagramSocket.java Sun Oct 11 03:13:39 2009
@@ -361,8 +361,8 @@
if (e.getMessage().equals(
"The socket does not support the operation")) { //$NON-NLS-1$
// receive packet to temporary buffer
- tempPack = new DatagramPacket(new byte[pack.capacity],
- pack.capacity);
+ tempPack = new DatagramPacket(new byte[pack.getCapacity()],
+ pack.getCapacity());
impl.receive(tempPack);
// tempPack's length field is now updated, capacity is unchanged
// let's extract address & port
@@ -404,11 +404,11 @@
.getOffset(), tempPack.getLength());
// we shouldn't update the pack's capacity field in order to be
// compatible with RI
- pack.length = tempPack.length;
+ pack.setLengthOnly(tempPack.getLength());
pack.setAddress(tempPack.getAddress());
pack.setPort(tempPack.getPort());
} else {
- pack.setLength(pack.capacity);
+ pack.setLength(pack.getCapacity());
impl.receive(pack);
// pack's length field is now updated by native code call;
// pack's capacity field is unchanged
@@ -442,7 +442,7 @@
} else {
// not connected so the target address is not allowed to be null
if (packAddr == null) {
- if (pack.port == -1) {
+ if (pack.getPort() == -1) {
// KA019 Destination address is null
throw new NullPointerException(Msg.getString("KA019")); //$NON-NLS-1$
}
Re: svn commit: r824006 - in /harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net:
DatagramPacket.java DatagramSocket.java
Posted by Nathan Beyer <nb...@gmail.com>.
On Wed, Oct 14, 2009 at 4:17 AM, Tim Ellison <t....@gmail.com> wrote:
> On 13/Oct/2009 19:59, Jesse Wilson wrote:
>> On Mon, Oct 12, 2009 at 7:18 PM, Nathan Beyer <nb...@gmail.com> wrote:
>>
>>> Honestly, the synchronization probably isn't appropriate on any of the
>>> accessor/mutators - the granularity isn't correct. The locking
>>> probably needs to encompass the entire method on DatagramSocket while
>>> working on a particular DatagramPacket. Realistically, you don't want
>>> the packet to change while processing.
>>>
>>
>> A test shows that the RI uses synchronized. The following program doesn't
>> complete:
>>
>> public static void main(String[] args) throws Exception {
>> byte[] data = new byte[] { 1, 2, 3, 4, 5 };
>> final DatagramPacket datagramPacket = new DatagramPacket(data,
>> data.length);
>>
>> ExecutorService executorService = Executors.newFixedThreadPool(1);
>>
>> synchronized (datagramPacket) {
>> Future<byte[]> future = executorService.submit(new Callable<byte[]>()
>> {
>> public byte[] call() throws Exception {
>> return datagramPacket.getData();
>> }
>> });
>>
>> byte[] fromAnotherThread = future.get();
>> System.out.println(Arrays.toString(fromAnotherThread));
>> }
>> }
>>
>>
>> Unfortunately, the concurrency support is shallow and not useful. Despite
>> the fact that the RI has synchronized statements, it fails to defensively
>> copy the data array:
>>
>> public static void main(String[] args) throws Exception {
>> byte[] data = new byte[] { 1, 2, 3, 4, 5 };
>> final DatagramPacket datagramPacket = new DatagramPacket(data,
>> data.length);
>> System.out.println("arrays are defensively copied? " + (data !=
>> datagramPacket.getData()));
>> }
>>
>>
>> As a consequence, one thread could mutate the data array while another
>> thread is consuming it.
>>
>> Regardless, it looks like synchronized and no defensive copying is the key
>> to compatibility with the RI.
>
> Thanks Jesse,
>
> I can't imagine that any application would rely on the methods being
> synchronized (as opposed to the fields being consistent), but I agree
> that we should leave it alone for compatibility sake.
>
> Unfortunately there are quite a few places where the RI implementation
> leaves a bit to be desired -- which always makes it tempting to build a
> better Java :-)
In some circles, synchronized is just an implementation detail and not
a definition of the interface. I think that may be on the SCJP test.
We could consider that unless the synchronization characteristics are
defined by the documentation, then don't have to abide by the same
implementation details.
-Nathan
>
> Regards,
> Tim
>
Re: svn commit: r824006 - in /harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net:
DatagramPacket.java DatagramSocket.java
Posted by Tim Ellison <t....@gmail.com>.
On 13/Oct/2009 19:59, Jesse Wilson wrote:
> On Mon, Oct 12, 2009 at 7:18 PM, Nathan Beyer <nb...@gmail.com> wrote:
>
>> Honestly, the synchronization probably isn't appropriate on any of the
>> accessor/mutators - the granularity isn't correct. The locking
>> probably needs to encompass the entire method on DatagramSocket while
>> working on a particular DatagramPacket. Realistically, you don't want
>> the packet to change while processing.
>>
>
> A test shows that the RI uses synchronized. The following program doesn't
> complete:
>
> public static void main(String[] args) throws Exception {
> byte[] data = new byte[] { 1, 2, 3, 4, 5 };
> final DatagramPacket datagramPacket = new DatagramPacket(data,
> data.length);
>
> ExecutorService executorService = Executors.newFixedThreadPool(1);
>
> synchronized (datagramPacket) {
> Future<byte[]> future = executorService.submit(new Callable<byte[]>()
> {
> public byte[] call() throws Exception {
> return datagramPacket.getData();
> }
> });
>
> byte[] fromAnotherThread = future.get();
> System.out.println(Arrays.toString(fromAnotherThread));
> }
> }
>
>
> Unfortunately, the concurrency support is shallow and not useful. Despite
> the fact that the RI has synchronized statements, it fails to defensively
> copy the data array:
>
> public static void main(String[] args) throws Exception {
> byte[] data = new byte[] { 1, 2, 3, 4, 5 };
> final DatagramPacket datagramPacket = new DatagramPacket(data,
> data.length);
> System.out.println("arrays are defensively copied? " + (data !=
> datagramPacket.getData()));
> }
>
>
> As a consequence, one thread could mutate the data array while another
> thread is consuming it.
>
> Regardless, it looks like synchronized and no defensive copying is the key
> to compatibility with the RI.
Thanks Jesse,
I can't imagine that any application would rely on the methods being
synchronized (as opposed to the fields being consistent), but I agree
that we should leave it alone for compatibility sake.
Unfortunately there are quite a few places where the RI implementation
leaves a bit to be desired -- which always makes it tempting to build a
better Java :-)
Regards,
Tim
Re: svn commit: r824006 - in /harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net:
DatagramPacket.java DatagramSocket.java
Posted by Jesse Wilson <je...@google.com>.
On Mon, Oct 12, 2009 at 7:18 PM, Nathan Beyer <nb...@gmail.com> wrote:
> Honestly, the synchronization probably isn't appropriate on any of the
> accessor/mutators - the granularity isn't correct. The locking
> probably needs to encompass the entire method on DatagramSocket while
> working on a particular DatagramPacket. Realistically, you don't want
> the packet to change while processing.
>
A test shows that the RI uses synchronized. The following program doesn't
complete:
public static void main(String[] args) throws Exception {
byte[] data = new byte[] { 1, 2, 3, 4, 5 };
final DatagramPacket datagramPacket = new DatagramPacket(data,
data.length);
ExecutorService executorService = Executors.newFixedThreadPool(1);
synchronized (datagramPacket) {
Future<byte[]> future = executorService.submit(new Callable<byte[]>()
{
public byte[] call() throws Exception {
return datagramPacket.getData();
}
});
byte[] fromAnotherThread = future.get();
System.out.println(Arrays.toString(fromAnotherThread));
}
}
Unfortunately, the concurrency support is shallow and not useful. Despite
the fact that the RI has synchronized statements, it fails to defensively
copy the data array:
public static void main(String[] args) throws Exception {
byte[] data = new byte[] { 1, 2, 3, 4, 5 };
final DatagramPacket datagramPacket = new DatagramPacket(data,
data.length);
System.out.println("arrays are defensively copied? " + (data !=
datagramPacket.getData()));
}
As a consequence, one thread could mutate the data array while another
thread is consuming it.
Regardless, it looks like synchronized and no defensive copying is the key
to compatibility with the RI.
Re: svn commit: r824006 - in /harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net:
DatagramPacket.java DatagramSocket.java
Posted by Nathan Beyer <nb...@gmail.com>.
On Mon, Oct 12, 2009 at 6:47 AM, Tim Ellison <t....@gmail.com> wrote:
> With a simple getting being synchronized like this, I assume it is to
> get the memory consistency on that field?
>
Yep. There were a two or three fields.
> Looking at DatagramPacket, all of the methods are synchronized.
> Wouldn't it be better to make these fields volatile, rather than
> requiring the setters and getters to acquire the lock as part of
> synchronization too?
I was just taking the conservative approach. Most of the existing code
called accessors on DatagramPacket already. It looks like at one point
that only accessors were used, but then some change was implemented to
access an internal-only field (capacity) and to access another
(length) without causing capcity to change.
Honestly, the synchronization probably isn't appropriate on any of the
accessor/mutators - the granularity isn't correct. The locking
probably needs to encompass the entire method on DatagramSocket while
working on a particular DatagramPacket. Realistically, you don't want
the packet to change while processing.
-Nathan
>
> I realize that some methods that access more than one field will still
> need to be synchronized.
>
> Regards,
> Tim
>
> On 11/Oct/2009 04:13, ndbeyer@apache.org wrote:
>> Author: ndbeyer
>> Date: Sun Oct 11 03:13:39 2009
>> New Revision: 824006
>>
>> URL: http://svn.apache.org/viewvc?rev=824006&view=rev
>> Log:
>> add some package methods to provide consistent synchronization of access to instance fields
>>
>> Modified:
>> harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net/DatagramPacket.java
>> harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net/DatagramSocket.java
>>
>> Modified: harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net/DatagramPacket.java
>> URL: http://svn.apache.org/viewvc/harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net/DatagramPacket.java?rev=824006&r1=824005&r2=824006&view=diff
>> ==============================================================================
>> --- harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net/DatagramPacket.java (original)
>> +++ harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net/DatagramPacket.java Sun Oct 11 03:13:39 2009
>> @@ -215,6 +215,15 @@
>> }
>>
>> /**
>> + * Gets the current capacity value.
>> + *
>> + * @return the current capacity value
>> + */
>> + synchronized int getCapacity() {
>> + return capacity;
>> + }
>> +
>> + /**
>> * Sets the length of the datagram packet. This length plus the offset must
>> * be lesser than or equal to the buffer size.
>> *
>> @@ -230,6 +239,19 @@
>> }
>>
>> /**
>> + * An alternative to {@link #setLength(int)}, that doesn't reset the {@link #capacity}
>> + * field.
>> + *
>> + * @param len the length of this datagram packet
>> + */
>> + synchronized void setLengthOnly(int len) {
>> + if (0 > len || offset + len > data.length) {
>> + throw new IllegalArgumentException(Msg.getString("K002f")); //$NON-NLS-1$
>> + }
>> + length = len;
>> + }
>> +
>> + /**
>> * Sets the port number of the target host of this datagram packet.
>> *
>> * @param aPort
>>
>> Modified: harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net/DatagramSocket.java
>> URL: http://svn.apache.org/viewvc/harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net/DatagramSocket.java?rev=824006&r1=824005&r2=824006&view=diff
>> ==============================================================================
>> --- harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net/DatagramSocket.java (original)
>> +++ harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net/DatagramSocket.java Sun Oct 11 03:13:39 2009
>> @@ -361,8 +361,8 @@
>> if (e.getMessage().equals(
>> "The socket does not support the operation")) { //$NON-NLS-1$
>> // receive packet to temporary buffer
>> - tempPack = new DatagramPacket(new byte[pack.capacity],
>> - pack.capacity);
>> + tempPack = new DatagramPacket(new byte[pack.getCapacity()],
>> + pack.getCapacity());
>> impl.receive(tempPack);
>> // tempPack's length field is now updated, capacity is unchanged
>> // let's extract address & port
>> @@ -404,11 +404,11 @@
>> .getOffset(), tempPack.getLength());
>> // we shouldn't update the pack's capacity field in order to be
>> // compatible with RI
>> - pack.length = tempPack.length;
>> + pack.setLengthOnly(tempPack.getLength());
>> pack.setAddress(tempPack.getAddress());
>> pack.setPort(tempPack.getPort());
>> } else {
>> - pack.setLength(pack.capacity);
>> + pack.setLength(pack.getCapacity());
>> impl.receive(pack);
>> // pack's length field is now updated by native code call;
>> // pack's capacity field is unchanged
>> @@ -442,7 +442,7 @@
>> } else {
>> // not connected so the target address is not allowed to be null
>> if (packAddr == null) {
>> - if (pack.port == -1) {
>> + if (pack.getPort() == -1) {
>> // KA019 Destination address is null
>> throw new NullPointerException(Msg.getString("KA019")); //$NON-NLS-1$
>> }
>>
>>
>>
>
Re: svn commit: r824006 - in /harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net:
DatagramPacket.java DatagramSocket.java
Posted by Tim Ellison <t....@gmail.com>.
With a simple getting being synchronized like this, I assume it is to
get the memory consistency on that field?
Looking at DatagramPacket, all of the methods are synchronized.
Wouldn't it be better to make these fields volatile, rather than
requiring the setters and getters to acquire the lock as part of
synchronization too?
I realize that some methods that access more than one field will still
need to be synchronized.
Regards,
Tim
On 11/Oct/2009 04:13, ndbeyer@apache.org wrote:
> Author: ndbeyer
> Date: Sun Oct 11 03:13:39 2009
> New Revision: 824006
>
> URL: http://svn.apache.org/viewvc?rev=824006&view=rev
> Log:
> add some package methods to provide consistent synchronization of access to instance fields
>
> Modified:
> harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net/DatagramPacket.java
> harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net/DatagramSocket.java
>
> Modified: harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net/DatagramPacket.java
> URL: http://svn.apache.org/viewvc/harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net/DatagramPacket.java?rev=824006&r1=824005&r2=824006&view=diff
> ==============================================================================
> --- harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net/DatagramPacket.java (original)
> +++ harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net/DatagramPacket.java Sun Oct 11 03:13:39 2009
> @@ -215,6 +215,15 @@
> }
>
> /**
> + * Gets the current capacity value.
> + *
> + * @return the current capacity value
> + */
> + synchronized int getCapacity() {
> + return capacity;
> + }
> +
> + /**
> * Sets the length of the datagram packet. This length plus the offset must
> * be lesser than or equal to the buffer size.
> *
> @@ -230,6 +239,19 @@
> }
>
> /**
> + * An alternative to {@link #setLength(int)}, that doesn't reset the {@link #capacity}
> + * field.
> + *
> + * @param len the length of this datagram packet
> + */
> + synchronized void setLengthOnly(int len) {
> + if (0 > len || offset + len > data.length) {
> + throw new IllegalArgumentException(Msg.getString("K002f")); //$NON-NLS-1$
> + }
> + length = len;
> + }
> +
> + /**
> * Sets the port number of the target host of this datagram packet.
> *
> * @param aPort
>
> Modified: harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net/DatagramSocket.java
> URL: http://svn.apache.org/viewvc/harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net/DatagramSocket.java?rev=824006&r1=824005&r2=824006&view=diff
> ==============================================================================
> --- harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net/DatagramSocket.java (original)
> +++ harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net/DatagramSocket.java Sun Oct 11 03:13:39 2009
> @@ -361,8 +361,8 @@
> if (e.getMessage().equals(
> "The socket does not support the operation")) { //$NON-NLS-1$
> // receive packet to temporary buffer
> - tempPack = new DatagramPacket(new byte[pack.capacity],
> - pack.capacity);
> + tempPack = new DatagramPacket(new byte[pack.getCapacity()],
> + pack.getCapacity());
> impl.receive(tempPack);
> // tempPack's length field is now updated, capacity is unchanged
> // let's extract address & port
> @@ -404,11 +404,11 @@
> .getOffset(), tempPack.getLength());
> // we shouldn't update the pack's capacity field in order to be
> // compatible with RI
> - pack.length = tempPack.length;
> + pack.setLengthOnly(tempPack.getLength());
> pack.setAddress(tempPack.getAddress());
> pack.setPort(tempPack.getPort());
> } else {
> - pack.setLength(pack.capacity);
> + pack.setLength(pack.getCapacity());
> impl.receive(pack);
> // pack's length field is now updated by native code call;
> // pack's capacity field is unchanged
> @@ -442,7 +442,7 @@
> } else {
> // not connected so the target address is not allowed to be null
> if (packAddr == null) {
> - if (pack.port == -1) {
> + if (pack.getPort() == -1) {
> // KA019 Destination address is null
> throw new NullPointerException(Msg.getString("KA019")); //$NON-NLS-1$
> }
>
>
>