CODEWITHSUNDEEP
javamultithreadingconcurrency
andy
andy

Reputation: 131

Synchronized Methods

If I have a synchronized public method and a private method:

public synchronized void doSomething() {
    doSomethingElse()
}

private void doSomethingElse() {
}

Do I need to synchronize the private method?

Upvotes: 13

Views: 13809

Answers (9)

Nathan Hughes
Nathan Hughes

Reputation: 96385

Even though the code works correctly when the private method is not synchronized, it would seem prudent from a maintainability perspective to make the private method synchronized.

Intrinsic locks are re-entrant, there is no harm in adding the synchronized keyword to the private method.

Putting code in a private method invites letting it be called by other methods, so it would make sense to make the private method synchronized in case in the future it is called from another method that does not otherwise need to be synchronized.

Upvotes: 0

J T
J T

Reputation: 5126

NO: if the only way doSomethingElse() is called is through another method that IS synchronized.

Possibly YES: If you call doSomethingElse() in some other fashion, through a method that is not synchronized, and you need to protect it against concurrent access.

Upvotes: 5

NateTheGreat
NateTheGreat

Reputation: 2305

It depends on what you're doing. Do you need to ensure serial access to doSomethingElse()?

If so, and the only thing calling doSomethingElse() is doSomething(), then no, you don't need to synchronize. But if some other method can call doSomethingElse(), then yes, you should synchronize it also.

Upvotes: 5

Jed Wesley-Smith
Jed Wesley-Smith

Reputation: 4706

This is the intent of the @GuardedBy annotation. If you expect that a lock must be held when calling that method, annotate it with that and the name of the lock (in the example it would be:

@GuardedBy("this") private void doSomethingElse() {…}

Then you can check that the invariant is true with FindBugs.

You can also use the other net.jcip.annotations for describing the thread-safety or lack of it and have FindBugs validate these assumptions too. Of course, the book needs a plug as well.

Upvotes: 3

Matt Wonlaw
Matt Wonlaw

Reputation: 12443

If you synchronize a block of code then anything called from within that block of code (on the same thread) still holds the initial lock. So doSomethingElse is still a part of the synchronized block when it is called from doSomething.

If you did:

public synchronized void doSomething() {
  new Thread() {
    public void run() {
      doSomethingElse();
    }
  }.start();
}
private void doSomethingElse() {
}

Then doSomethingElse would not have the lock that was acquired by doSomething.

Also avoid synchronized methods as it exposes the implementation details of your concurrency policy. See this question on synchronized(this) / synchronized methods: Avoid synchronized(this) in Java?

If doSomethingElse must be synchronized, regardless of whether or not it is called from doSomething, it wouldn't hurt to synchronize doSomethingElse as synchronized locks are re-entrant (i.e., if a thread already has a lock on an object it can get the lock again).

Upvotes: 1

Danish
Danish

Reputation: 3798

Although what you've done is fine, in my opinion, synchronization should be done at the lowest granularity possible. Which would suggest to synchronize the actual private function. Currently, you're assuming that no other function in the class will call the private function independently. This may not be the case in the future.

Upvotes: 0

NPE
NPE

Reputation: 500167

It depends:

  • If doSomethingElse is safe to call concurrently, then you don't need synchronized.
  • If it isn't, then the answer depends on where it is called from:
    • If it is only called from other synchronized methods, then it doesn't need to be synchronized (but marking it as such will do no harm);
    • If it can be called from methods that aren't synchronized themselves, then it must be synchronized.

Upvotes: 19

Nick Banks
Nick Banks

Reputation: 4408

Personally, I do not like synchronized methods. I prefer to synchronize with some kind of lock varaible, as so:

private final Object lock = new Object();

public void doSomething() {
  synchronized(lock) {
    // Do some safely
    doSomethingElse();
  }
  // Do some work un-safely
}

private void doSomethingElse() {
  // Do some work safely because still in lock above
  // ...
}

Upvotes: 0

Ted Hopp
Ted Hopp

Reputation: 234795

Any methods called from a synchronized method (or from within a synchronized block) are run while still synchronized. You don't need to separately synchronize the private method if it is only called from synchronized methods.

Upvotes: 2

Related Questions