• Post Reply Bookmark Topic Watch Topic
  • New Topic
programming forums Java Mobile Certification Databases Caching Books Engineering Micro Controllers OS Languages Paradigms IDEs Build Tools Frameworks Application Servers Open Source This Site Careers Other Pie Elite all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Campbell Ritchie
  • Jeanne Boyarsky
  • Ron McLeod
  • Paul Clapham
  • Liutauras Vilda
Sheriffs:
  • paul wheaton
  • Rob Spoor
  • Devaka Cooray
Saloon Keepers:
  • Stephan van Hulst
  • Tim Holloway
  • Carey Brown
  • Frits Walraven
  • Tim Moores
Bartenders:
  • Mikalai Zaikin

good or bad

 
Ranch Hand
Posts: 443
3
Eclipse IDE C++ Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
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 ]
 
author and iconoclast
Posts: 24207
46
Mac OS X Eclipse IDE Chrome
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I agree that it's almost without a doubt a mistake.
 
Wanderer
Posts: 18671
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
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 ]
 
Ranch Hand
Posts: 291
Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
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.
 
Ranch Hand
Posts: 387
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

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 ]
 
With a little knowledge, a cast iron skillet is non-stick and lasts a lifetime.
reply
    Bookmark Topic Watch Topic
  • New Topic