• Post Reply Bookmark Topic Watch Topic
  • New Topic

Could my program fail with race condition by concurrent threads in a multi thread environment?  RSS feed

 
Kumar Raja
Ranch Hand
Posts: 550
2
Hibernate Java Spring
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hello All,

I have a task of returning US holiday based on the given date and this utility will be used in a multi thread environment. I have written as below, could some one take a look and advise, if my code breaks at any given point by multiple threads

Declared Enum




Client code will call


I tried to test this with few concurrent threads, and noticed that the DB call is made for the very first time or when the year being requested is not same as the cached year. But I wanted to see, if this is properly synchronizing and would not fail in any case.

Any suggestions, feedback. Did I over complicate it?

My intention is to make a singleton HolidayCalendar, and also synchronized well enough so that every thread using this class gets the required data without blocking each other.
 
Kumar Raja
Ranch Hand
Posts: 550
2
Hibernate Java Spring
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I'm also thinking, may be I don't have to use a singleton Enum here. Instead I could make HolidayCalendar with static methods and synchronize on DB call.
 
Tim Holloway
Saloon Keeper
Posts: 18800
74
Android Eclipse IDE Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Static is not thread-safe. To be thread-safe, you have to be synchronized over the scope of all affected objects/properties.


 
Mike Simmons
Ranch Hand
Posts: 3090
14
  • Likes 2
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Some quick observations:

Holiday.holidayName needs to be final, to ensure it will never be observed in uninitialized state (null) by any thread.

HolidayCalendar.getHoliday() has insufficient synchronization. I assume that cacheHolidays() populates the holidays instance field, right? But you don't use that field until after leaving the sync block - which means another thread could come along and set holidays to the holidays for some other year. Which defeats the purpose. The simplest fix is to move holidays.get(...) inside the sync block. Alternately, you could use a local variable to retain a reference to holidays - set this variable inside the sync block - and then use the local variable outside the block. That way if another thread changes the field, you're safe; you still have a reference to the holidays for the year you're interested in.

There may be more issues, but those are the two that jumped out to me.

Is this overcomplicated? Well, probably. Do you know if there's actually a performance issue with simpler code, e.g. no caching at all? There's a good chance you don't need any of this concurrency stuff here. If you do, well, this might be OK. But I would consider caching more than just one year. What happens if it's near the end of the year, and you frequently need to do lookups of two different years? To save yourself some time and headaches, I would look at using Guava's caching utilities to handle this.
 
Mike Simmons
Ranch Hand
Posts: 3090
14
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Tim Holloway wrote:Static is not thread-safe.

I think that's misleading - using static is neither more nor less thread-safe than using an enum singleton. Thread safety is an orthogonal issue to static vs. singleton. Either way, if you have mutable state (whether in static variables or instance variables on a singleton, as in the case under discussion, or any other kind of state) then you need to synchronize (or use other concurrency countermeasures) to protect that state from concurrency issues. The static vs singleton issue may affect how you synchronize (on what monitor), but it doesn't affect whether you should synchronize.

Tim Holloway wrote:To be thread-safe, you have to be synchronized over the scope of all affected objects/properties.

Agreed.
 
Jayesh A Lalwani
Rancher
Posts: 2762
32
Eclipse IDE Spring Tomcat Server
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
The way you have now, each time the year switches, you will introduce a bottleneck.. and you have no control over when the year switches. You might make your code "Safe", but that doesn't mean that it's "Good". Heavy use of synchronization makes your code safe, but it limits scalability. It's better to not have synchronized at all.. or if you do limit the time you spend in synchronized blocks.

It's much better to design it so that when a thread is using a object that is heavy and slow to create, no other thread has the same instance OR an immutable instance is shared between threads

Whenever I've had to do caching like this, I've either used a thread local variable or a pool of instances.

1) Thread Local variable

This will work only if you are not creating threads willy nilly. If you are running in a container then chances are the container will use a thread pool. Also, if you are using ExecutorService, then you are probably using a thread pool. Basically what you do is instead of having a singleton, use a thread local variable. Do this



This makes your code rock solid without needing any synchronization. You don;t need to synchronize any of the code inside the HolidayCalendar because every instance of your class can be called by one and only thread. Performance wise, this scales well too, because the JVM just keeps the reference to the object in the thread's context, and it can look it up without any need for synchronization. The disadvantages here are (again), it will perform poorly if you keep starting and stopping threads, and is a better fit for thread pools. ALso, obviously, you have an instance per thread.. so if you have large number of threads, you will have lots of instances. You shouldn't be creating lots of threads and pooling your threads anyways, so this option might work ... most of the time

2) Use an Object pool

I would use this solution if you have no guarantee that you will be called on a thread pool. AN Object pool is just a generic version of Connection Pool. If you know what Connection Pool is then you know what Object pool is. Basically, it;s a "pool" of instances. Whenever you need an instance, you borrow from the pool. When you are done you return back to the pool. When another thread asks for an instance from the pool, it gets a different instance. Again no 2 instances share the same object. So, no need to synchronize within the object. The only point, synchronization is required is when an object is retreived from the pool.

The advantage here of course is that you can control the number of instances. It doesn't default to the number of threads. So, for really heavy objects you can limit the instances. The disadvantage is greater complexity of code:- You have to make sure you return the object to the pool. If you have a bug, you might run into starvation.

Apache Commons provides a neat implementation of a pool called Apache Commons pool. You can use GenericObjectPool (Actually DBCP is implemented using Apache Commons) . The pool implementation provided by Apache is doing the synchronization. So, you don;t need to synchronize in your "real" code at all. All the synchronization is limited to a lookup in the pool.
ALternatively, you can use KeyedObjectPool with the year as a key. This provides a good LRU implementation right out of the box. You can keep calendars for multiple years in your pool. When the pool gets full, the pool will start ejecting LRU instances.

Personally, I tend to use thread local variables, because I don;t let my threads go wild, unless I run into an a problem that requires a really heavy object.
 
Kumar Raja
Ranch Hand
Posts: 550
2
Hibernate Java Spring
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thank you all.
 
Consider Paul's rocket mass heater.
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!