harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Nathan Beyer <nbe...@gmail.com>
Subject Re: svn commit: r824006 - in /harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/net: DatagramPacket.java DatagramSocket.java
Date Tue, 13 Oct 2009 02:18:42 GMT
On Mon, Oct 12, 2009 at 6:47 AM, Tim Ellison <t.p.ellison@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$
>>                  }
>>
>>
>>
>

Mime
View raw message