Synchronized code block

Question

Why do people "synchronize" for just 1 line of code? What is there to "synchronize"?

public final void addListener(Listener listener) {
  synchronized (listeners) {
    listeners.add(listener);
  }
}

EDIT: Thank you everyone. Very good answers to from all!

Answer 1

synchronized on its own means that if multiple threads try to run this piece of code at the same time, only one of those threads is allowed inside the block at any given time. synchronized (listeners) uses listeners as a lock identifier, which means that this restriction applies to all blocks which synchronize on that variable - if one thread is inside one of those blocks, no other thread may enter any of them.

Even though there's only a single function call in a block, this can still make sense: that function consists of a lot of other instructions, and control may switch to a different thread while the first one is in the middle of that function. If the function is not thread-safe, that can cause problems, such as data getting overwritten.

In this particular case, the function call consists of adding a value to a collection listeners. While it's not impossible to make a thread-safe collection, most collections are not thread-safe for multiple writers. Thus, in order to ensure the collection does not get messed up, synchronized is needed.

EDIT: To give an example of how things may get messed up, assume this simplified implementation of add, where length is the number of elements in the items array:

public void Add(T item) {
  items[length++] = item;
}

That length++ bit is not atomic; it consists of a read, an increment, and a write, and the thread can get interrupted after any of them. So, let's rewrite this a bit, to see what's really happening:

public void Add(T item) {
  int temp = length;
  length = length + 1;
  items[temp] = item;
}

Now assume two threads T1 and T2 enter Add at the same time. Here's one possible set of events:

T1: int temp = length;
T2: int temp = length;
T2: length = length + 1;
T2: items[temp] = item;
T1: length = length + 1;
T1: items[temp] = item;

The problem there is that the same value is used for temp by both threads, so the last thread to leave ends up overwriting the item that the first one put there; and there's an unassigned item at the very end.

It also doesn't help if length represented the next index to be used so we can use a preincrement:

public void Add(T item) {
  items[++length] = item;
}

Again, we rewrite this:

public void Add(T item) {
  length = length + 1;
  items[length] = item;
}

Now this is a possible sequence of events:

T1: length = length + 1;
T2: length = length + 1;
T2: items[length] = item;
T1: items[length] = item;

Once again, the last thread ends up overwriting the first, but now the unassigned item is the second-to-last item.