GeeCON Prague 2014*
The moose likes Threads and Synchronization and the fly likes good or bad Big Moose Saloon
  Search | Java FAQ | Recent Topics | Flagged Topics | Hot Topics | Zero Replies
Register / Login


JavaRanch » Java Forums » Java » Threads and Synchronization
Bookmark "good or bad" Watch "good or bad" New topic
Author

good or bad

Chris Hurst
Ranch Hand

Joined: Oct 26, 2003
Posts: 417
    
    2

Genius or Madness ... bug or feature

I've inherited some code, no comments, no idea of programmer ability can't talk to either but two seperate programmers
appear to be following 'an interesting' pattern.


They're basically locking a member variable then changing the reference used to locate the locked object within the synchronized block,
i.e. I exepcetd them to lock on the same object but they don't necessarily.


Object m_state = state1; // where state1, state2 etc are static final objects created elsewhere


in one public member code of this form ...

and in another public member code of this form ...

My problem was that without comments a quick glance the intention of the code wasn't obvious (I suspect its not what the author intended at all)
and led to a whole list of further evils, too many to list and what they actually intended I suspect could probably be achieved in one of many far more simpler options.


Now I'm not sure if the author intended what they have written but would it be reasonable to say this in a code review ...


1) If your not an expert in java threading never change the reference to the variable you sync on i.e. introduce a new object to lock on without making it very obvious.
2) If you are an expert comment your code to warn others and explain what the intention of the synchronization is i.e. what it does and what it
doesn't.



i.e. my feeling is such code is bordering on evil even if intentional without comments , or are you all doing it ;-).
,
[ October 11, 2006: Message edited by: Jim Yingst ]

"Eagles may soar but weasels don't get sucked into jet engines" SCJP 1.6, SCWCD 1.4, SCJD 1.5,SCBCD 5
Ernest Friedman-Hill
author and iconoclast
Marshal

Joined: Jul 08, 2003
Posts: 24187
    
  34

I agree that it's almost without a doubt a mistake.


[Jess in Action][AskingGoodQuestions]
Jim Yingst
Wanderer
Sheriff

Joined: Jan 30, 2000
Posts: 18671
Yep. Bad craziness. Can't think of any good "expert" reason to do anything like this. Well, maybe if only one thread ever changes the reference, it might work, but... ewww. Much, much more likely to be a mistake on the part of the original coder(s).

Chris, I addded code tags to your message above. I recommend using them in the future for readability.
[ October 11, 2006: Message edited by: Jim Yingst ]

"I'm not back." - Bill Harding, Twister
Edward Harned
Ranch Hand

Joined: Sep 19, 2005
Posts: 291

Not every programmer is sane. That's what makes life interesting.

m_state is not an object but a reference to an object. So if m_state == state1, a thread will sync on the state1 object. If that thread changes the m_state reference to state2, another thread will sync on the state2 object. If these threads run concurrently, they may step on each other since they hold different monitors.

Either the author was nuts or he intentionally introduced a nasty code bomb for someone else to experience.

If the latter, he should be taken out at dawn and shot.


Ed's latest article: A Java Parallel Calamity http://coopsoft.com/ar/Calamity2Article.html
Dan Bizman
Ranch Hand

Joined: Feb 25, 2003
Posts: 387
Originally posted by Chris Hurst:
Genius or Madness ... bug or feature



Yuck.

I could be wrong, but I think (judging by the name "state") they meant to do something more like:



but they probably thought they were being "clean" and "efficient" by using one variable/object to do two things.
[ October 12, 2006: Message edited by: Dan Bizman ]
 
I agree. Here's the link: http://aspose.com/file-tools
 
subject: good or bad