Static method in multithreading

Question

I have the following code in my class

private static final SimpleDateFormat SDF_ISO_DATE = new SimpleDateFormat("yyyy-MM-dd");
private static final SimpleDateFormat SDF_ISO_TIME = new SimpleDateFormat("HH:mm:ss");

public static String getTimeStampAsString(final long time) {
    TimeZone tz = TimeZone.getTimeZone("UTC");
    SDF_ISO_DATE.setTimeZone(tz);
    SDF_ISO_TIME.setTimeZone(tz);
    return SDF_ISO_DATE.format(
        new Date(time)) + " " + SDF_ISO_TIME.format(new Date(time)
    );
}

In my multi threaded application the following method returns date in future, even for the current date, is the static method or variable is responsible for this?

edit:

I had the following code to reproduce and prove what are mentioned in the answers,but still not able to.Can some one help me for the same.

public static void main(String[] args) throws InterruptedException, ExecutionException {

            Callable<String> task = new Callable<String>(){
                public String call() throws Exception {
                    return DateUtil.getTimeStampAsString(1524567870569L);
                }
            };

            //pool with 50 threads
            ExecutorService exec = Executors.newFixedThreadPool(50);
            List<Future<String>> results = new ArrayList<Future<String>>();

            //perform 10 date conversions
            for(int i = 0 ; i < 50 ; i++){
                results.add(exec.submit(task));
            }
            exec.shutdown();
            //look at the results
            for(Future<String> result : results){
                System.out.println(result.get());
            }
    }

Answer 1

is the static method or variable is responsible for this?

Static variables. SimpleDateFormat isn't thread-safe, which should be obvious since you're modifying its internal state by calling setTimeZone(). It means that several threads could be doing that at the same time, which should feel like producing unpredictable results.

You need to build your formats locally rather than reuse some defined statically. Or better yet, drop Java's old time-managing classes and use java.time.* instead.

Answer 2

tl;dr

To capture the current moment and generate a string in your desired format (which is a modified form of standard ISO 8601 format), use the java.time classes. These classes are much simpler and vastly better designed. They are also thread-safe.

Instant.now().toString().replace( "T" , " " )

Current moment

Your method is named getCurrentTimeStamp(final Date date) yet you are passing an existing Date object set to a specific moment rather than capturing the current moment.

Nowhere in your code do I see you capturing the current moment. If you want the current moment, call Instant.now() as shown below.

Avoid legacy date-time classes

The legacy date-time classes such as Date & SimpleDateFormat are not thread-safe. One of many reasons to avoid these troublesome classes. They were supplanted years ago by the java.time classes.

java.time

As a moment in UTC, the java.util.Date class is replaced by the Instant class. Same idea, but Instant has a resolution in nanoseconds rather than milliseconds. And Instant::toString does not inject a time zone dynamically as Date::toString does.

To capture the current moment in UTC, call the static Instant.now() method.

Instant instant = Instant.now() ;  // Capture current moment in UTC.

Parse your input number as a count of milliseconds since the epoch reference of first moment of 1970 in UTC.

Instant instant = Instant.ofEpochMilli( 1_524_567_870_569L ) ;

instant.toString(): 2018-04-24T11:04:30.569Z

No need for must of your code. No need for your DateUtil, as seen in code above. No need for custom formatting patterns, as your desired format happens to comply with the ISO 8601 standard used by default in the java.time classes. If the T in the middle bothers you or your users, replace with a SPACE.

String output = instant.toString().replace( "T" , " " ) ;

2018-04-24T11:04:30.569Z

ExecutorService blocking

You seem to misunderstand ExecutorService::shutdown. That method does not block to wait for tasks to complete. As your code is written, some tasks may not yet be done running until after you report results (partially-completed results).

Add a call to ExecutorService::awaitTermination, as seen in code below. Set a time-out long enough that if exceeded it must mean some problem occurred. To quote the doc:

Block until all tasks have completed execution after a shutdown request, or the timeout occurs, or the current thread is interrupted, whichever happens first.

See example code below. For more discussion see this Question, ExecutorService - How to wait for completition of all tasks in non-blocking style

Threads

The java.time classes are thread-safe by design. They use the immutable objects pattern, returning fresh object based on existing values rather than changing (“mutating”) the original.

Example code. Your Question is confused about whether you want a hard-coded moment or the current moment. Switch to either by enabling the commented-out line in this example.

Callable < String > task = new Callable < String >() {
    public String call () throws Exception {
        long threadId = Thread.currentThread().getId();

// String moment = Instant.ofEpochMilli( 1524567870569L ).toString().replace( "T" , " " ); String moment = Instant.now().toString().replace( "T" , " " ); String output = ( moment + " | " + threadId ); return output; } };

// Pool with 5 threads
ExecutorService exec = Executors.newFixedThreadPool( 5 );
List < Future < String > > results = new ArrayList < Future < String > >();

// Perform a certain number of tasks.
int countAssignedTasks = 500;
for ( int i = 0 ; i < countAssignedTasks ; i++ ) {
    results.add( exec.submit( task ) );
}

// Wait for tasks to complete.
Boolean completedBeforeTimeOut = null;
try {
    exec.shutdown();
    completedBeforeTimeOut = exec.awaitTermination( 5 , TimeUnit.SECONDS );  // Block until all tasks have completed execution after a shutdown request, or the timeout occurs, or the current thread is interrupted, whichever happens first.
} catch ( InterruptedException e ) {
    e.printStackTrace();
}

// Report results.
System.out.println( "completedBeforeTimeOut: " + completedBeforeTimeOut );
for ( Future < String > result : results ) {
    try {
        System.out.println( result.get() );
    } catch ( InterruptedException e ) {
        e.printStackTrace();
    } catch ( ExecutionException e ) {
        e.printStackTrace();
    }
}

System.out.println( "BASIL - done." );

When run.

Note that the times are not chronological. In multi-threaded code, you cannot predict which tasks will be executed when.

2018-04-24 20:24:06.991225Z | 13

2018-04-24 20:24:06.991246Z | 14

2018-04-24 20:24:06.991236Z | 15

2018-04-24 20:24:06.991232Z | 16

2018-04-24 20:24:06.991222Z | 17

2018-04-24 20:24:07.067002Z | 16

2018-04-24 20:24:07.067009Z | 17

Answer 3

As an answer to your edit: how to reproduce the problem with thread-unsafety (not sure whether that really ought to be a separate question). Formatting the same date in two or more threads using the same SimpleDateFormat seems to go well (at least most often, no guarantee that it always will). Try formatting different date-times, and it will be very easy to get wrong results. I changed your task like this:

    AtomicLong time = new AtomicLong(1_524_567_870_569L);

    Callable<String> task = new Callable<String>(){
        @Override
        public String call() {
            return DateUtil.getTimeStampAsString(time.getAndAdd(2_768_461_000L));
        }
    };

It’s easiest to see that the results are wrong when I also sort them in the output, so I have done that. I am only quoting the first few results from one run since this is enough to demonstrate the problem:

2018-04-24 11:04:30
2018-05-26 12:05:31
2018-06-11 13:06:32
2018-07-29 14:07:33
2018-08-08 15:08:34
2018-10-01 16:09:35
…

The expected result was (obtained by declaring getTimeStampAsString() synchronized; also sorted afterward):

2018-04-24 11:04:30
2018-05-26 12:05:31
2018-06-27 13:06:32
2018-07-29 14:07:33
2018-08-30 15:08:34
2018-10-01 16:09:35
…

Already the fifth printed result has the day-of-month all wrong, 08 instead of 30, and there are many more errors in the full list. You may try it yourself. As you probably know, exact results are not reproducible, but you should get results that are wrong somehow.

PS Here’s my code for printing the results in sorted order in case you want to try it:

    //look at the results
    SortedSet<String> sorted = new TreeSet<>();
    for (Future<String> result : results){
        sorted.add(result.get());
    }
    sorted.forEach(System.out::println);

Answer 4

tz is effectively constant and the setters don't do anything after the first invocation of either method. Use a static initialiser to set the timezone right away to make the methods thread-safe.

private static final SimpleDateFormat SDF_ISO_DATE = new SimpleDateFormat("yyyy-MM-dd");
private static final SimpleDateFormat SDF_ISO_TIME = new SimpleDateFormat("HH:mm:ss");

static {
    TimeZone tz = TimeZone.getTimeZone("UTC");
    SDF_ISO_DATE.setTimeZone(tz);
    SDF_ISO_TIME.setTimeZone(tz);
}

public static String getCurrentTimeStamp(final Date date) {
    return SDF_ISO_DATE.format(date) + " " + SDF_ISO_TIME.format(date);
}

public static String getTimeStampAsString(final long time) {
    return getCurrentTimeStamp(new Date(time));
}