1

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()); } } 
5
  • 3
    SimpleDateFormat is not thread safe. You'll have to synchronise your method. Commented Apr 24, 2018 at 8:49
  • 1
    Yes, synchronization would be necessary. But the better solution, if you always set to UTC, would be to set this one time only (More efficient, anyway.) in a static initializer block. Even better: Use Java 8's time package. (DateTimeFormatter and LocalDateTime or ZonedDateTime) Commented Apr 24, 2018 at 8:51
  • Based on this answer, the class itself is not thread safe. I think it has nothing to do with the setTimeZone call. And date and time format can be set together. Commented Apr 24, 2018 at 9:06
  • 1
    I recommend you avoid the SimpleDateFormat class. It is not only long outdated, it is also notoriously troublesome. Today we have so much better in java.time, the modern Java date and time API and its DateTimeFormatter. Which BTW is threadsafe. Commented Apr 24, 2018 at 9:15
  • Your comment says 5 threads, but your code says 50 (not 5): Executors.newFixedThreadPool(50) Commented Apr 24, 2018 at 20:31

4 Answers 4

5

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.

Sign up to request clarification or add additional context in comments.

2 Comments

but several threads will be doing the same thing in the same time r8?
@masSdev sorry I don't understand what you mean. When will they? With existing code or with my suggestions? And if they receive different dates as parameters, they won't do the same thing
1

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); 

Comments

1

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

Comments

0

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)); } 

3 Comments

That's not thread-safe. SimpleDateFormat maintains an internal state when doing its work, and multiple threads are not supposed to modify it at the same time. It would produce unpredictable results.
@Michael you misunderstand. SimpleDateFormat does its work with an internal state. It's not nearly limited to just whatever is the TimeZone it's supposed to use. Everything it needs to keep as working variables to do its formatting/parsing is part of its internal state. Multiple threads cannot be touching that at the same time, it won't work, or at least it makes no attempt to work.
@OleV.V. Huh. So it does. The poor design decisions of the old date/time stuff never fails to amaze. In which case, kumesana is right, it's not used in a thread-safe way and this answer requires additional synchronization.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.