tags:

views:

684

answers:

9

I am attempting to troubleshoot an intermittent failure that appears to be related to removing an object from a HashMap and then putting that same object back using a new key. My HashMap is created as follows:

transactions = new HashMap<Short, TransactionBase>();

The code that does the re-assignment is as follows:

transactions.remove(transaction.tran_no);
transaction.tran_no = generate_transaction_id();
transactions.put(transaction.tran_no, transaction);

The intermittent behaviour that I am seeing is that code that executes immediately after this that depends upon the transaction object being locatable does not appear to find the transaction object using the new transaction id. However, at some future point, the transaction can be located. So pulling at straws, is there any sort of asynchronous effect to put() or remove that can cause this sort of behaviour?

I should mention that, to the best of my knowledge, the container is being accessed by only one thread. I have already read in he documentation that class HashMap is not "synchronised".

+1  A: 

So you know HashMap's not thread safe. Are you sure it's being accessed by only one thread? After all, intermittent failures a frequently threading related. If not, you can wrap it with Collections.synchronizedMap(), like so:

Collections.synchronizedMap(transactions);

You could always just try so you can eliminate that possibility.

It should be pointed out that this just wraps the original map with one with all the methods synchronized. You may want to consider using a synchronized block if the access is localized.

sblundy
This won't work because he's performing 3 separate operations on the map. He needs a synchronized block around all 3 to ensure that they atomic, and then synchronize again in his getter.
Outlaw Programmer
Probably a good idea
sblundy
If you use Collections.synchronizedMap() it exposes the lock it uses for internal operations so that you can build these 3-step transactions without having to add extra synchronization for one-step operations. See the javadoc.
Darron
Can you provide a specific link? Looking at the source code it looks like the SynchronizedMap class is package private and, even if it wasn't, the mutex it uses isn't exposed anywhere. This is the Java 1.6 source, by the way.
Outlaw Programmer
+4  A: 

There are no "asynchronous" effects in the HashMap class. As soon as you put something in there, it's there. You should double- and triple- check to make sure that there are no threading issues.

The only other thing I can think of is that you're making a copy of the HashMap somewhere. The copy obviously won't be affected by you adding stuff into the original, or vice versa.

Outlaw Programmer
+7  A: 

There is a slight difference between remove/get and put (although my guess is that you have a threading issue).

The parameter for remove/get is of type Object; for put it is of type K. The reason for that has been stated many times before. This means it has problems with boxing. I'm not even going to guess what the rules are. If a value gets boxed as a Byte in one place and a Short in another, then those two objects cannot be equal.

There is a similar issue with List.remove(int) and List.remove(Object).

Tom Hawtin - tackline
+6  A: 

I presume that every time you check for the presence of the item you're definitely using a short or Short argument to Map.get() or Map.contains()?

These methods take Object arguments so if you pass them an int it will be converted to an Integer and will never match any item in your Map because they will all have Short keys.

Adrian Pronk
Very good point.
Michael Myers
+2  A: 

Just one suggestion...you're focusing on accesses to the HashMap but I wonder if you should also see if your generate_transaction_id() is thread safe or if it is behaving in an unexpected way.

Michael Sharek
+2  A: 

Have you overridden equals() but not hashCode() in your model objects? How about compareTo() ? If you get these wrong, Collections will behave strangely indeed.

Check Java Practices on equals() and compareTo().

Limbic System
The HashMap is using Short as the key, so it shouldn't be an issue, is that correct or am I missing something?
Michael Sharek
Yes you're correct,it shouldn't be a problem if he's using Short as the key.
Limbic System
+1  A: 

What does this generate_transaction_id() function do? If it is generating a 16-bit GUID-like thing, you could easily get hash collisions. Combined with threading, you could get:

T1: transaction1.tran_no = generate_transaction_id();    => 1729
T2: transaction2.tran_no = generate_transaction_id();    => 1729
T1: transactions.put(transaction1.tran_no, transaction1); => map.put(1729, transaction1)
T2: transactions.put(transaction2.tran_no, transaction2); => map.put(1729, transaction2)
T1: int tran_no = transactions.get(1729);               => transaction2
T1: transactions.remove(transaction.tran_no);           => map.remove(1729)
T2: int tran_no = transactions.get(1729);               => null

Of course, this could only be a solution if that 'to the best of your knowledge' part is not true.

A: 

Threading has been mentioned in a few responses already, but have you considered the visibility issue for objects used by multiple threads? It's possible (and quite common) that if you put an object into a collection in one thread, it will not be "published" to other threads unless you have properly synchronized on the collection.

Threads and Locks

Synchronization and thread safety in Java

Limbic System
A: 

As others have observed you HAVE to know whether the HashMap is accessed by just one Thread, or not. CollectionSpy is a new profiler that lets you find out instantly, for all containers, how many threads perform any accesses. See www.collectionspy.com for more details.

Laurence Vanhelsuwe