Hashmap and multiple threads

[Hoss Spence]

Hi,

I inherited some code that uses a Hashmap being accessed and
updated by multiple threads in a completely unprotected
(unsynchronized) manner. I discovered this after looking at a JBOSS
thread dump that showed all ten threads in this state.

JBOSS Thread Dump
Thread: JMS SessionPool Worker-68 : priority:5, demon:true, threadId:
1786, threadState:RUNNABLE, threadLockName:null

java.util.HashMap.containsKey(Unknown Source)

com.ingenix.freya.rulesengine.ListServiceSingleton.getListTypeByName
(ListServiceSingleton.java:77)
com.ingenix.freya.rulesengine.RulesKBServiceImpl.getListTypeByName
(RulesKBServiceImpl.java:2884)


Although not protecting the Hashmap operations is clearly wrong, it
doesn't explain to me why all threads seemed to be in the containsKey
() call. Does anyone have any ideas? This is hard to duplicate (as
you'd expect a problem with using a non synchronized Hashmap accessed
by multiple threads would be).

Also I had originally thought to fix this by synchronizing just around
the "put" but now am wondering if this should be done at the "get()"
and "containsKey()" code as well. Any thoughts on this?

 

[Lew]

I inherited some code that uses a Hashmap [sic] being accessed and

Spelling counts.

updated by multiple threads in a completely unprotected
(unsynchronized) manner. I discovered this after looking at a JBOSS
thread dump that showed all ten threads in this state.

JBOSS Thread Dump
Thread: JMS SessionPool Worker-68 : priority:5, demon:true, threadId:
1786, threadState:RUNNABLE, threadLockName:null

java.util.HashMap.containsKey(Unknown Source)

com.ingenix.freya.rulesengine.ListServiceSingleton.getListTypeByName
(ListServiceSingleton.java:77)
com.ingenix.freya.rulesengine.RulesKBServiceImpl.getListTypeByName
(RulesKBServiceImpl.java:2884)


Although not protecting the Hashmap [sic] operations is clearly wrong, it
doesn't explain to me why all threads seemed to be in the containsKey

Luck of the draw. Apparently the users of the HashMap have a usage pattern
involving a lot of 'containsKey()' calls.

() call. Does anyone have any ideas? This is hard to duplicate (as
you'd expect a problem with using a non synchronized Hashmap accessed
by multiple threads would be).

Also I had originally thought to fix this by synchronizing just around
the "put" but now am wondering if this should be done at the "get()"
and "containsKey()" code as well. Any thoughts on this?

You cannot get away with synchronizing half the uses. You must synchronize
all of them. You should study multi-threaded programming before you accept a
paycheck for this work. Read /Java Concurrency in Practice/ by Brian Goetz,
et al., and read every IBM DeveloperWorks article by Mr. Goetz before you
begin. "A little knowledge is a dangerous thing."

That you even considered synchronizing only the 'put()' calls indicates that
you don't know what you need to know.

You can wrap the Map in a 'Collections.synchronizedMap()' call if the
synchronization need is simple, or perhaps use a ConcurrentHashMap.

Declare the variable as a 'Map', not as the implementing type.

 

[Mark Space]

Although not protecting the Hashmap operations is clearly wrong, it
doesn't explain to me why all threads seemed to be in the containsKey
() call. Does anyone have any ideas? This is hard to duplicate (as

I also find it very suspicious that you managed to get 10 threads inside
a single call. That's like winning the super mega lotto. Are you sure
these threads are unsynchronized? It would make more sense if all these
threads were blocked and waiting on the same lock.

Are you sure you have an actual HashMap and not some synchronized
object? The latter would make more sense.

Anyway, Lew's right, multi-threading is tricky and should be studied
carefully before making attempts to "fix" things. Simply using a call like

Map h = Collections.synchronizedMap( new HashMap() );

to create this HashMap will protect your map, but it won't necessarily
protect your code. A strong analysis of the code is probably in order
to determine if there are more serious issues.

 

[Hoss Spence]

   I inherited some code that uses a Hashmap [sic] being accessed and

Spelling counts.
Jeesh

updated by multiple threads in a completely unprotected
(unsynchronized) manner. I discovered this after looking at a JBOSS
thread dump that showed all ten threads in this state.

JBOSS Thread Dump
Thread: JMS SessionPool Worker-68 : priority:5, demon:true, threadId:
1786, threadState:RUNNABLE, threadLockName:null

    java.util.HashMap.containsKey(Unknown Source)

com.ingenix.freya.rulesengine.ListServiceSingleton.getListTypeByName
(ListServiceSingleton.java:77)
    com.ingenix.freya.rulesengine.RulesKBServiceImpl.getListTypeByName
(RulesKBServiceImpl.java:2884)

Although not protecting the Hashmap [sic] operations is clearly wrong, it
doesn't explain to me why all threads seemed to be in the containsKey

Luck of the draw.  Apparently the users of the HashMap have a usage pattern
involving a lot of 'containsKey()' calls.

I don't think it's luck, there is only one place in the code where
this is done (I should have made that clear)... and the 10 threads are
all hitting it. That's the main question. Has anyone seen a Map get
into a state where all the threads appear to be blocking in it (even
though according to the Thread dump all threads are in the Runnable
state).

You cannot get away with synchronizing half the uses.  You must synchronize
all of them.  You should study multi-threaded programming before you accept a
paycheck for this work.  Read /Java Concurrency in Practice/ by Brian Goetz,
et al., and read every IBM DeveloperWorks article by Mr. Goetz before you
begin.  "A little knowledge is a dangerous thing."

I have the Goetz book and I do plan to use Collections.synchronizedMap
() once I wrap a junit test around it. The reason I asked the
synchronize everywhere question is that performance is a very big
issue here (I can test that with a functional test). Obviously there
needs to be trade offs and protection comes first. While I intend to
fix the synchronization issue I can't be sure it's actually causing
the problem which is making all the threads time out in this call.
There is no easy way to test things like this so will just have to
employ best thread safe-guard practices I guess.

Declare the variable as a 'Map', not as the implementing type.

Yes... that was one of the many things wrong with the code.

Thanks for your response.

 

[Hoss Spence]

Thanks for responding. Comments below.

I also find it very suspicious that you managed to get 10 threads inside
a single call.  That's like winning the super mega lotto.  Are you sure
these threads are unsynchronized?  It would make more sense if all these
threads were blocked and waiting on the same lock.

Are you sure you have an actual HashMap and not some synchronized
object?  The latter would make more sense.

That was my first thought. I looked through the code in all cvs
versions and I did not see any synchronization calls at all. Nor had
the Map itself been synchronized.
Very bizarre.

Anyway, Lew's right, multi-threading is tricky and should be studied
carefully before making attempts to "fix" things.  Simply using a call like

   Map h = Collections.synchronizedMap( new HashMap() );

to create this HashMap will protect your map, but it won't necessarily
protect your code.  A strong analysis of the code is probably in order
to determine if there are more serious issues.

It is unfortunate that in the times we live in (we've had multiple
layoffs here and much of our stuff is offshored) there is very little
time to devout to making our programming staff more proficient in
multi threaded practices. I'm still trying to push better junit
coverage as well as Checkstyle. I appreciate your comments and agree
with them.

 

[Mark Space]

synchronize everywhere question is that performance is a very big
issue here (I can test that with a functional test). Obviously there
needs to be trade offs and protection comes first. While I intend to

You should go back to Java Concurrency in Practice and re-read chapter
three, on sharing data, especially the parts about visibility. In the
absence of a lock, changes made on one thread just aren't visible to
other threads. It's not about protection or performance, it's about
getting it to work at all, other than by accident.

All methods that change the Map must be synchronized somehow, or the
changes aren't visible.

 

[Hoss Spence]

You should go back to Java Concurrency in Practice and re-read chapter
three, on sharing data, especially the parts about visibility.  In the
absence of a lock, changes made on one thread just aren't visible to
other threads.  It's not about protection or performance, it's about
getting it to work at all, other than by accident.

All methods that change the Map must be synchronized somehow, or the
changes aren't visible.

Just curious, you would think that the threads would be in a WAIT
state if they were waiting on a lock not in the RUNNABLE state they
are shown to be in. That's what makes me think the Map got corrupted
and motivated me to ask if others have ever seen that.

Anyway will just correct the obvious problems and hope for the best.
Thanks again.
My thought would be the MAP is corrupted and it's causing the

 

[Hoss Spence]

...

Another thought, depending on how your application is driven. Suppose
there is some transaction that requires a containsKey call that is going
to go into a loop because of a broken linked list. That transaction is
given to one thread, which gets captured, and fails to respond. After
some time-out, the transaction is resubmitted, picked up by a different
worker thread, which does the containsKey call and gets captured. That
continues, until all worker threads are attempting the same, impossible
to complete, operation.

Note that HashMap, in order to perform better than LinkedList, needs
most of its operations to not access any given bucket. Threads go on
working normally until they access the bucket with the broken list, and
there may be only the one transaction that needs to do that access, and
its first access to the bucket is a containsKey call.

Patricia

Hi Patricia... nice to see you are still posting here. Your scenario
makes sense. Looks like I'm going to have to go further down the
rabbit hole to see what's going on here. The joys of inheriting code
from ghosts of programmers past.

 

[Lew]

I would *not* do selective synchronization. It could leave you with
flakiness that is even harder to reproduce and deal with than your
current problem. It might prevent the lists from becoming permanently
broken, but still cause a e.g. a get call to fail to find an entry that
really is there, because it scanned a list while it was being modified
by another thread.

Or it could cause the get to fail because there is no /happens-before/
relationship between the put and the get, even though the put
completes prior to the get chronologically.

Mark Space hit the nail on the head when he mentioned visibility.
There is no guarantee that writes from one thread are visible to reads
from another thread, absent synchronization.

 

[Lew]

It is unfortunate that in the times we live in (we've had multiple
layoffs here and much of our stuff is offshored) there is very little
time to devout to making our programming staff more proficient in
multi threaded practices.

You might present your management with this question:
Which is better, to devote time to getting multi-threaded programming correct
(since your program apparently is multi-threaded), or to spend much, much more
time and money fixing the subtle, maddening threading bugs after you've
already dismayed your customers with them?

I've worked on a project where management ignored the cries to fix Java
threading issues until after a customer got the wrong data because of a data
race. Oh, then you see a lot of stürm und drang, let me tell you. War rooms
and tiger teams and late nights, lots of high-level pressure from top
management and executives, finger-pointing, chest-beating, falling on swords, ...

It's not pretty.

 

[Albert]

Hoss Spence a écrit :

Hi,

I inherited some code that uses a Hashmap being accessed and
updated by multiple threads in a completely unprotected
(unsynchronized) manner. I discovered this after looking at a JBOSS
thread dump that showed all ten threads in this state.

JBOSS Thread Dump
Thread: JMS SessionPool Worker-68 : priority:5, demon:true, threadId:
1786, threadState:RUNNABLE, threadLockName:null

java.util.HashMap.containsKey(Unknown Source)

com.ingenix.freya.rulesengine.ListServiceSingleton.getListTypeByName
(ListServiceSingleton.java:77)
com.ingenix.freya.rulesengine.RulesKBServiceImpl.getListTypeByName
(RulesKBServiceImpl.java:2884)


Although not protecting the Hashmap operations is clearly wrong, it
doesn't explain to me why all threads seemed to be in the containsKey
() call. Does anyone have any ideas? This is hard to duplicate (as
you'd expect a problem with using a non synchronized Hashmap accessed
by multiple threads would be).

Also I had originally thought to fix this by synchronizing just around
the "put" but now am wondering if this should be done at the "get()"
and "containsKey()" code as well. Any thoughts on this?

I would say, if you didn't find a real bug, but only suspicious things,
don't touch anything, until you get a real bug...

 

[Hoss Spence]

You might present your management with this question:
Which is better, to devote time to getting multi-threaded programming correct
(since your program apparently is multi-threaded), or to spend much, much more
time and money fixing the subtle, maddening threading bugs after you've
already dismayed your customers with them?

I've worked on a project where management ignored the cries to fix Java
threading issues until after a customer got the wrong data because of a data
race.  Oh, then you see a lot of stürm und drang, let me tell you.  War rooms
and tiger teams and late nights, lots of high-level pressure from top
management and executives, finger-pointing, chest-beating, falling on swords, ...

It's not pretty.

Lew,

I've already had that conversation today and I recently got some
positive feedback going forward. I am going to plow into the Goetz
book, become the "threads expert", look for unprotected static Maps in
the code, fix them and do a Brown Bag with development. This of course
with the 10 million other things I'm tasked with. By the way the multi-
threading is done in the JMS container using MDB's in Jboss, so the
application developers are somewhat oblivious to that fact that we are
even in a multi-threading environment as they just see their little
piece. Seems like elementary practice that you protect anything Static
to me, but it may not be obvious to a 21 year old from India, so some
training is in order. I think Patricia's linked list (in the Map)
theory and the visibility issues of not synchronizing are the most
likely culprit here. Thanks again for your help.

 

[DrJ]

Hoss,

I should also note, in case you are unaware, that doing any sort of
synchronization on an application server such as JBoss is against the
EJB spec. That is not to say you can't do it, only that you
shouldn't. You should be aware that this makes your app non-portable
and prevents you from using clustering.

That being said, due to the architecture of my project, I have to deal
with many similar issues and so I have been using synchronization in
the app server and it has worked like a charm. If you intend on ever
changing your container though, you need to be aware of the potential
issues.

Good luck!

-Chris

 

[Hoss Spence]

Hoss,

I should also note, in case you are unaware, that doing any sort of
synchronization on an application server such as JBoss is against the
EJB spec.  That is not to say you can't do it, only that you
shouldn't.  You should be aware that this makes your app non-portable
and prevents you from using clustering.

That being said, due to the architecture of my project, I have to deal
with many similar issues and so I have been using synchronization in
the app server and it has worked like a charm.  If you intend on ever
changing your container though, you need to be aware of the potential
issues.

Good luck!

-Chris

Chris,

Good point. I will investigate this, it is not part of the of the
stateless EJB being used, just being called down the stack so I'm not
sure if that applies or not. The Map is being used for caching and
there are better ways to do this using third party caching products.
This will be part of my research.