CODEWITHSUNDEEP
javamultithreadingsynchronization
dmonopoly
dmonopoly

Reputation: 3331

Is making a method synchronized when the method is only called in a synchronized block redundant? What happens on the OS level?

Let's say you have a method foo() that is only called in a synchronized block.

Is making the method

synchronized void foo() {..}

versus just

void foo() {..}

redundant or bad practice, or does it make the method's intent clearer? Does having the synchronized keyword on the method signature make the program slower? I'm wondering what the better practice is, or if I'm missing part of the big picture.

Thanks.

Upvotes: 3

Views: 89

Answers (3)

CurtainDog
CurtainDog

Reputation: 3205

It's not redundant and in practice it's reasonably common to synchronise on locks you already hold.

APIs that have clearly defined boundaries between what's threadsafe and what's not are easier to work with, and the best boundary is probably at the class level. So, if the rest of the class is threadsafe and the foo() method will be threadsafe if synchronised then you may as well do it and make a note of it in your class doc. If synchronising foo() will give you an API with mixed threadsafe and non-threadsafe calls then that's going to lead to confusion later on.

Regarding performance I doubt it'll be an issue (uncontested locks should have minimal overhead). Get the code working and then see if you need to make it faster.

Upvotes: 1

lipido
lipido

Reputation: 66

I imagine your foo() method is in a class A, and the caller code (which calls it inside a synchronized block) is in another class B.

My advice is not to make methods of A synchronized, because in single-threaded programs invocations will be systematically slower.

It is better to make the client classes (such as B) to carry out the synchronization job, by using synchronized blocks, as you pointed out.

In summary, my advice is to not declare the method synchronized, because it is more flexible: you can make thread-safe programs using your class with external synchronized blocks and also make fast single-threaded applications. You can also warn your users that your class A is not thread-safe.

Upvotes: 0

irreputable
irreputable

Reputation: 45433

Usually, this means the foo() is not an atomic action itself, it probably does not preserve invariants, it is only part of the grander synchronization scheme.

Then it should not declare synchronized, otherwise reader might get the wrong idea; and it is a bit slower too.

Instead, you could

void foo()
{
    assert Thread.holdsLock(this);
    ...
}

it serves 2 purposes

  1. document that this method can only be invoked within an outer synchonized(this) block
  2. check that requirement at runtime (I'll enable assersion for all non-production runs)

Upvotes: 1

Related Questions