I’ll cover each point separately.
-
Some evil code may steal your lock (very popular this one, also has an
“accidentally” variant)I’m more worried about accidentally. What it amounts to is that this use of
this
is part of your class’ exposed interface, and should be documented. Sometimes the ability of other code to use your lock is desired. This is true of things likeCollections.synchronizedMap
(see the javadoc). -
All synchronized methods within the same class use the exact same
lock, which reduces throughputThis is overly simplistic thinking; just getting rid of
synchronized(this)
won’t solve the problem. Proper synchronization for throughput will take more thought. -
You are (unnecessarily) exposing too much information
This is a variant of #1. Use of
synchronized(this)
is part of your interface. If you don’t want/need this exposed, don’t do it.