• Post Reply Bookmark Topic Watch Topic
  • New Topic

synchronization question

 
Marilyn de Queiroz
Sheriff
Posts: 9079
12
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I have a question about the following code:


Originally I was thinking that it was a bad idea to change the object, hash, that I'm synchronizing on, but then I thought that since it's the last thing done inside the synchronized code, it's probably ok. However, it seems like the get() method should be synchronized as well since it is accessing the same object, hash, and that object might be changing simultaneously with the method call (hash.get()).

I would appreciate it if someone could validate my conclusion or alternatively explain to me where I'm getting confused.

Thanks.
 
Ernest Friedman-Hill
author and iconoclast
Sheriff
Posts: 24213
35
Chrome Eclipse IDE Mac OS X
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
My feeling is that reportAndClear() is fine, because for the duration of the method, you're locking one specific Hashtable and manipulating only that one Hashtable. The fact that you change the "hash" member on the way out doesn't change the above.

As far as synchronizing your get() method: you not only don't need to, but you can remove the existing synchronization on the put() method. That's because both Hashtable.put() and Hashtable.get() are already synchronized methods.

My comment holds if getMessageId() doesn't touch "hash", which I'm assuming it doesn't. If it does touch "hash", then I'd say that both lines of the get() method may belong in a block synchronized on the Hashtable.

One seeming issue is that a call to put() may be blocking and waiting for reportAndClear() to return; when it does return, it will lock the old Hashtable, but I believe it will operate on the new one. But because Hashtable.put() is itself synchronized, this is harmless. If you remove the synchronization, then it might operate on the old Hashtable, which could result in a put() effectively getting lost. Not sure if this is a problem for you or not, but I guess it's an argument for leaving put() the way it is.
 
Jim Yingst
Wanderer
Sheriff
Posts: 18671
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
[EFH]: I guess it's an argument for leaving put() the way it is.

I see it as an argument for synchronizing on something else entirely, for all three methods.

The issue EFH cites in the last paragraph of his post is a potential problem even if another thread isn't blocking until the report finishes. Even if it's "after" report finishes, the other thread may see the old value of the hash ariable, and operate on the old Hashtable. Because the access to the hash variable itself is not synchronized - only the accesses to the Hashtable referenced by hash are synchronized. There's a real danger of losing a put() here.

This is the sort of thing that may work correctly 90% of the time, or 99%, or 99.99%. Or it may work perfectly, until one day it runs on a different sstem and suddenly it's at 90%. And it's the sort of thing that can be a real pain to debug later because it's so sporadic. So I recommend avoiding the possibility. The simple way is to have all the methods (including the get() sync on the same object - an unchanging reference. Having them all sync on "this" (the MessageHash) would be the standard approach. Or a private final instance of some sort.
[ May 24, 2006: Message edited by: Jim Yingst ]
 
Marilyn de Queiroz
Sheriff
Posts: 9079
12
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Actually, I was getting a NullPointerException and I suspected this section of code. I thought perhaps the hash would be getting updated at the same time that the (LogMessage)hash.get(messageId) was getting done.

However, the log file was 1GB and I deleted it. So I don't have the exact error message anymore. (I should have saved just that section, 20-20 hindsight)
 
Jim Yingst
Wanderer
Sheriff
Posts: 18671
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
[MdQ]: I thought perhaps the hash would be getting updated at the same time that the (LogMessage)hash.get(messageId) was getting done.

I don't think that's a possibility for the code you've shown. Since every time code accesses a Hashtable, it is synchronized on that Hashtable. (In the get(), the sync is internal.) The problem is that the variable hash is not part of the Hashtable, and is accessed several times without any sync. So bad craziness could result, but I think the NullPointerexception is being caused by something else. Which doesn't mean you shouldn't also fix the sync problem here.
 
Ernest Friedman-Hill
author and iconoclast
Sheriff
Posts: 24213
35
Chrome Eclipse IDE Mac OS X
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
It's always dangerous to disagree with Jim, because the guy is scary smart, but I'm going to anyway. Note that I'm talking about the new memory model here because, well, the old one was broken -- although the relevant stuff hasn't actually changed, as far as I know.

If you disassemble put() as written, you get, in part,



So we fetch the "hash" field, dup it, save a copy in local #4, and lock the Hashtable. Then we go back out to memory to read Hashtable field again and get a new copy, which we then call Hashtable.put() on. Then we fetch the copy we made in local #4, and unlock it.

The Java memory model says that if a thread makes some changes to memory in a synchronized block and then unlocks the object, then a subsequent thread that locks that same object is guaranteed to see all the same changes. So if reportAndClear() locks Hashtable #1 and then stores Hashtable #2 in "hash", and while reportAndClear() is running in the synchronized block put() is called, then one of two things can happen: either put() locks #1 and changes #2, or it locks #2 and changes #2, too (if the memory changes take effect before the synchronized block is exited.) It is actually not possible that when put() blocks on #1 because of reportAndClear() but then stores into #1.

OK, Jim, now tell me why I'm wrong.
 
Marilyn de Queiroz
Sheriff
Posts: 9079
12
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
When did the memory model change? At this point, we're still using jdk1.3
 
Jim Yingst
Wanderer
Sheriff
Posts: 18671
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
[MdQ]: When did the memory model change? At this point, we're still using jdk1.3

1.5. Like EFH, I don't think the changes are actually relevant here. But I could've missed something.

[EFH]: It is actually not possible that when put() blocks on #1 because of reportAndClear() but then stores into #1.

All right, it looks like you're right on that point - I was wrong to say the put() would operate on the old Hashtable. It would operate on the newer Hashtable. Assuming there are only two of interest at a time, a questionable assumption. Could be that a third thread has called report, and changed the hash variable after our last read, and we'd still have a lost put.

Or how about this:

Thread A calls report(), and reads the value of hash, getting a ref to Hashtable 1. Pause...

Thread B calls report(), also reads Hashtable 1, locks it, rereads hash (again getting #1), iterates to completion, replaces hash with #2, and unlocks hash #1. Done with report().

Thread A resumes, locks #1, rereads hash, gets #2, gets an iterator, and starts iterating #2 without having a lock on it..

Thread B calls put(), locks #2, and does a put() - redundantly locking #2 again. Let's say this causes a resize.

Thread A sees a ConcurrentModificationException if it's lucky. If not though, iterating during a resize could yield a number of possible random errors depending on where in the code the two threads actually intersect.

It's possible that if calls to report() are rare enough, or if they're always done by the same thread (never concurrently), this code might be safe. But that sounds pretty iffy to me, and it seems very easy to just sync everything on something else (something final) instead.
 
Ernest Friedman-Hill
author and iconoclast
Sheriff
Posts: 24213
35
Chrome Eclipse IDE Mac OS X
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Fair enough. Concurrent calls to reportAndClear() are a disaster. Sounds like you're right about a separate lock.
 
Marilyn de Queiroz
Sheriff
Posts: 9079
12
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thank you, guys.
 
Mr. C Lamont Gilbert
Ranch Hand
Posts: 1170
Eclipse IDE Hibernate Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
That get method is absolutely bonkers. Without synchronization its not guaranteed to see any of the changes produced by reportAndClear. This is because reportAndClear changes the variable (not the internals of the hashtable), but that change is not guaranteed to be seen by any thread except one that synchronizes on the same monitor that the changer synchronized on.

Also with respect to the new memory model though I have argued about this, in the old model, being that you are accessing a variable (hash) without ever synchronizing on its assignment, that reference itself could still be null. In other words, the original assignment during construction of MessageHash is only guaranteed to be seen by the thread that created the MessageHash object. And this gets into all sorts of memory model stuff so best to leave it here.

get absolutely must be synchronized.

Also I agree on the repeated calls of reportAndClear between calls to put. Its an odd situation I have never seen before. But it appears that legally put is required only to sort of iterate over each assignment to hash one at a time. Yes it can skip to the latest, but its only required to see one change per synch. Which I find quite funny. Theres a unique way to create an iterator, lol.


let me be clear on what I am saying. reportAndClear can lock hash which is ObjectA, then change hash to ObjectB. Next time lock ObjectB, then change hash to ObjectC. Then lock ObjectC, then change hash to ObjectD, etc...

(Ignoring the null issue)
Some other thread calls put and locks on ObjectA, it is only required to see the assignment made under that lock which is hash=ObjectB. Next call to put this thread lock on ObjectB. And is only required to see the assignment that took place under that lock which is hash=ObjectC. etc...

So each other thread can have its only private iteration going on due to the memory model (old or new). And each put is lost because its being made always on old objects. I agree that the threads dont have to iterate and can skip. but the most they are legally required to do is iterate one at a time.
[ June 01, 2006: Message edited by: Mr. C Lamont Gilbert ]
 
Mr. C Lamont Gilbert
Ranch Hand
Posts: 1170
Eclipse IDE Hibernate Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Actually I dont think it will iterate like that. It does not matter what the lock is I don't think. Whenever it encounters a lock its going to refresh every object that lives within the scope of that lock IIRC. So the visibility is not an issue as I thought. Only issue is poor exclusivity and initialization.
 
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!