Synchronization syntax for access to concurrent collection

Question

I have the following code:

public class State {
    private List<Position> m_track;

    public State() {
        m_track = Collections.synchronizedList(new ArrayList<Position>());
    }

    public List<Position> getTrack() {
        return m_track;
    }
}

// in different class
public void modifyTrack(State _state) {
    List<Position> local_track = _state.getTrack();
    synchronized (local_track) {
        // safely modify track
    }
}

But Android Studio gives me a warning on line synchronized (local_track):

Synchronization on local variable 'local_track' Reports synchronization on a local variable or parameter.

It is very difficult to guarantee correctness when such synchronization is used.

It may be possible to improve code like this by controlling access through e.g. a synchronized wrapper class, or by synchronizing on a field.

If I replace synchronized (local_track) with synchronized (_state.getTrack()) warning goes away.

If I understand correctly my local_track is just a reference and no new object is created and if so why can't I synchronize on it?

And read about Java naming conventions. "_" is for SOME_CONSTANT only. You don't use it anywhere else. And that "m_" to indicate "its a field" is also discouraged by most java coding style guides. — GhostCat, May 10 '17 at 07:47

score 1 · Accepted Answer · answered May 10 '17 at 07:54

1

I agree with EJPs comment - in this case, it is safe to ignore the warning for the "multi-threading" aspect of it.

But: I still think the warning is a symptom of "bad practice". You see: you externalize that list to users of your Stateclass; and worse: and then that user class locks on that list; probably to then modify it.

That is the opposite of good OO. You make an internal implementation detail known to the public; and you even intend to modify that internal data structure!

In my eyes, this is violating various principles, such as TDA or simple "information hiding".

Thus: in response to this warning; you might want to step back and look at your design more closely; and see for example if there is a way to have the State class do those modifications to its track list!

answered May 10 '17 at 07:54

GhostCat

137,827
25
176
248

oops, actually, instead of "safely modify track" comment should be "safely iterate track" :) That way it seems like a tolerable design solution – Alexey Andronov May 10 '17 at 08:37
Even then: locking on that thing looks "wrong" to me. You might instead have a look [here](http://stackoverflow.com/questions/38845778/thread-safety-when-iterating-through-an-arraylist-using-foreach) and use a different kind of list that makes it **safe** to iterate it. – GhostCat May 10 '17 at 08:43
well, I replaced `Collections.synchronizedList` with `CopyOnWriteArrayList` and it seems like more simple and good-looking solution, thanks for your input and links – Alexey Andronov May 10 '17 at 09:18
1

You are very welcome! Glad I could tell you more than "ignore it" ;-) – GhostCat May 10 '17 at 09:21

Synchronization syntax for access to concurrent collection

1 Answers1