Interview Q: Java Synchronization

Question

I encountered this questions a couple of months ago while interviewing via Skype for a German company. Given the following code:

private static DateFormat DATE_FORMAT = new SimpleDateFormat();       
public void doSomething() {
    for (int i = 0; i < 100; i++) {
        new Thread(new Runnable() {
            @Override
            public void run() {
                synchronized (DATE_FORMAT) {
                    System.out.println(DATE_FORMAT.format(Calendar.getInstance().getTime()));
                }

            }
        }).start();
    }
}

State if there could be any potentially synchronization issues and why.

My intuition tells me that there shouldn't be any. We are creating 100 Threads, each of them will adquire the lock on the same object (DATE_FORMAT) and display the current time with more or less accuracy. However, I remember that the interviewer mentioned something about inconsistencies in the printing but I cannot recall correctly.

Thanks in advance.

Answer 1

I don't see a problem since there is a critical section that is controlled by the single monitor (DATE_FORMAT) and no other locks are present, so there is no risk of a deadlock.

The only thing I can think of is the DATE_FORMAT field not being final, so potentially other code could change the referent, but that would still not cause a problem since the main use of this is that you do not run format on the same instance of SimpleDateFormat concurrently.

Answer 2

According to given code;

Calling format method from DATE_FORMAT's instance is modifying the calender object which is in DATE_FORMAT's instance , so its possible to 1 thread may modify calender before other thread prints( other thread is the thread which modified the calender object but it didn't print yet) .

here is the reference in SimpleDateFormat.class

// Called from Format after creating a FieldDelegate
private StringBuffer format(Date date, StringBuffer toAppendTo,
                            FieldDelegate delegate) {

// Convert input date to time field list
calendar.setTime(date); // modifies the calender's instance

Okay, however the lock on DATE_FORMAT should prevent any other Thread to modify the calendar inside the the DATE_FORMAT no? – @Santi

It is not preventing to modify calendar directly but it is preventing accessing DATE_FORMAT's instance, If 2 thread tries to execute synchronized block at the same time with same argument(which is DATE_FORMAT's instance ) 1 thread should wait another to execute that synchronized block. Thats how synchronized works.

Now as i promised in the comment, i made simulation to prove my answer.

    private static DateFormat DATE_FORMAT = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS");     
    private static ArrayList<String> listDate  = new ArrayList<>();

    public static void doSomething() 
    {
        for (int i = 0; i < 100; i++) {

            final long msCurrentDate =   i*100;

            new Thread(new Runnable() {

                public void run() {
                   synchronized (DATE_FORMAT) {
                        listDate.add(DATE_FORMAT.format(new Date(msCurrentDate)));
                        //System.out.println(DATE_FORMAT.format(Calendar.getInstance().getTime()));
                    }
                }
            }).start();
        }


        Runtime.getRuntime().addShutdownHook(new Thread()
        {
            @Override
            public void run()
            {
                int resultSize = listDate.size();
                System.out.println("All elements' size :" + resultSize);
                resultSize = listDate.stream().distinct().collect(Collectors.toList()).size();
                System.out.println("Unique elements' size :" + resultSize);
            }
        });


    }

I modified the given code without changing it's purpose. As you can see i am using the fixed(and increasing 100ms for per thread) time to compare results with synced and not synced versions of code.

I am printing the Dates with and also adding Dates to the ArrayList of String to work with numbers, not just look and feel comparing.

First let me add the printed results :

On Left side there is 2 multiple Date printed, On Right side there is no multiple Date

of course first 5 results prove nothing, you have to check every one of them. So i added results to the List and printed results after removing same entries from list

Here is the results for Synced version :

//Output of all executions
//All elements' size :100
//Unique elements' size :100

Here is the results of Not Synced version:

//Output of execution : 1
//All elements' size :100
//Unique elements' size :82

//Output of execution : 2
//All elements' size :100
//Unique elements' size :78

//Output of execution : 3
//All elements' size :100
//Unique elements' size :81

According to results we can say that calendar is changing by X thread before A,B,C... threads printing the date(or adding to list)

You can test and see it yourself, for distinct result you need JDK 8 to use streams api or you can use any other code. Please let me know if you have any questions so we can argue.

Answer 3

You should probably use new SimpleDateFormat("HH:mm:ss.SSS") as your formatter, getting the desired weirdness with hours and minutes requires good timing to hit a minute boundary.

I'd say with confidence that there isn't any synchronization issues with the code as-is. Any concerns with thread-safety of the various calls are put to rest by the synchronization block, all the initialization seems in order, no particular gotchas really leap out at me.

What may have made sense is if the Calendar.getInstance().getTime() call was made/assigned outside of the synchronization block and used within it. The synchronization block will not necessarily call the threads awaiting the lock in the order that they arrive, leading to potentially out-of-order interleaving of the output, but that isn't the case with the current code. All I can suggest is that perhaps your interviewer presented the wrong code or they were mistaken.

For reference, the following code will produce out-of-order interleaving:

public class Test {
    private static DateFormat DATE_FORMAT = new SimpleDateFormat("HH:mm:ss.SSS");
    public static void main(String[] args) {
        for (int i = 0; i < 100; i++) {
            new Thread(new Runnable() {
                @Override
                public void run() {
                    Date time = Calendar.getInstance().getTime();
                    synchronized (DATE_FORMAT) {
                        System.out.println(DATE_FORMAT.format(time));
                    }
                }
            }).start();
        }
    }
}