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

[Peter Levart]

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