RFR: 6206780 (str) Forwarding append methods in String{Buffer, Builder} are inconsistent

[Jim Gish]

Thank you, Peter.

Alan -- could you please do a final review with the changes I made to 
the test and push it for me?

http://cr.openjdk.java.net/~jgish/Bug6206780-sb-append-forward/

Thanks,
    Jim

On 10/07/2012 06:17 PM, Peter Levart wrote:
> On 10/07/2012 08:42 PM, Alan Bateman wrote:
>> On 06/10/2012 01:52, Jim Gish wrote:
>>> I've revamped the previous proposed change.  Please review the 
>>> update at 
>>> http://cr.openjdk.java.net/~jgish/Bug6206780-sb-append-forward/ 
>>> <http://cr.openjdk.java.net/%7Ejgish/Bug6206780-sb-append-forward/>
>>>
>>> This revision contains the following additions/changes:
>>> 1. consistent usage of @Overrides
>>> 2. comments on unsynchronized methods depending on other 
>>> synchronized methods
>>> 3. overall more code coverage of insert, append, indexOf, 
>>> lastIndexOf, etc. for both StringBuffer and StringBuilder
>>> 4. testing of StringBuffer to ensure that all public un-synchronized 
>>> methods are actually synchronized along their call chain.  the 
>>> StringBuffer/TestSynchronization class uses reflection to decide 
>>> which methods to test and could be (with a bit of work on method 
>>> parameter setup), be used to test any class for these 
>>> synchronization properties.  The motivation for this test is to 
>>> provide some degree of assurance that modifications to StringBuffer, 
>>> particularly addition of new methods, do not break the 
>>> synchronization contract. The code also includes a self-test to 
>>> guard against breaking the test program itself.
>>>
>> I'm skimmed over this and StringBuffer looks a lot better, thanks for 
>> reverting back and adding the comments.
>>
>> I don't have time to review the tests just now but it looks like 
>> you've added a test of useful tests. I did look at 
>> TestSynchronization.testSynchronized and the approach looks right. 
>> The timeout is short so there will periodically be false positives 
>> but that's okay as increasing the timeout will slow down the test 
>> (the main thing I was looking for was the potential for false 
>> negatives).
>>
>> -Alan
>
> Hi Jim,
>
> Here's a way to test for synchronization deterministically. And it 
> takes as much time as the testing methods runs (no timeouts):
>
>
> public class SyncTest
> {
>     public static class MyClass
>     {
>         public synchronized void synchronized_method() throws 
> InterruptedException
>         {
>             Thread.sleep(1000);
>         }
>
>         public void not_synchronized_method() throws InterruptedException
>         {
>             Thread.sleep(1000);
>         }
>
>         public void indirectly_synchronized_method() throws 
> InterruptedException
>         {
>             Thread.sleep(100);
>             synchronized_method();
>         }
>     }
>
>     private static boolean isSynchronized(Method m, Object target)
>     {
>         Thread t = new Thread(new InvokeTask(m, target));
>
>         Boolean isSynchronized = null;
>
>         synchronized (target)
>         {
>             t.start();
>
>             while (isSynchronized == null)
>             {
>                 switch (t.getState())
>                 {
>                     case NEW:
>                     case RUNNABLE:
>                     case WAITING:
>                     case TIMED_WAITING:
>                         Thread.yield();
>                         break;
>                     case BLOCKED:
>                         isSynchronized = true;
>                         break;
>                     case TERMINATED:
>                         isSynchronized = false;
>                         break;
>                 }
>             }
>         }
>
>         try
>         {
>             t.join();
>         }
>         catch (InterruptedException ex)
>         {
>             ex.printStackTrace();
>         }
>
>         return isSynchronized;
>     }
>
>     public static void main(String[] args)
>     {
>         Method[] methods = MyClass.class.getDeclaredMethods();
>         Object target = new MyClass();
>
>         for (Method m : methods)
>         {
>             System.out.println(m + " - isSynchronized: " + 
> isSynchronized(m, target));;
>         }
>     }
>
>     static class InvokeTask implements Runnable
>     {
>         private final Method m;
>         private final Object target;
>         private final Object[] args;
>
>         InvokeTask(Method m, Object target, Object...args)
>         {
>             this.m = m;
>             this.target = target;
>             this.args = args;
>         }
>
>         @Override
>         public void run()
>         {
>             try
>             {
>                 m.invoke(target, args);
>             }
>             catch (Exception e)
>             {
>                 e.printStackTrace();
>             }
>         }
>     }
> }
>
>
>
> Regards, Peter
>

-- 
Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304
Oracle Java Platform Group | Core Libraries Team
35 Network Drive
Burlington, MA 01803
jim.gish at oracle.com