0

I wonder if a method is going to be thread safe if it just calls another thread safe method. I have an example like this. As the docs state, ConcurrentSkipListSet is thread-safe. 

public class MessageHolder {

    private Set<String> processedIds;

    public MessageHolder() {
        this.processedIds = new ConcurrentSkipListSet<>();
    }

    public void add (String id) {
        processedIds.add(id);
    }

    public boolean contains (String id) {
        return processedIds.contains(id);
    }

    public void remove (String id) {
        processedIds.remove(id);
    }

}

You may ask why I am not using ConcurrentSkipListSet directly. The reason is that I want to create an interface for the actions performed here and this example will be like an in memory version.

Mansur
  • 1,661
  • 3
  • 17
  • 41
  • 1
    This is not necessarily thread safe: it would not be thread safe to use, say, `if (!mh.contains(id)) { mh.add(id); }`, if you only want to call `add` when the id is not already present. – Andy Turner Jun 24 '19 at 11:04
  • What if there won't be a situation as such in my current setting, would it be still thread-unsafe? And I got the point of your example, but is there any other way to determine which problems can occur apart from just looking for the cases that will make problem specifically in my current setting. – Mansur Jun 24 '19 at 11:23
  • You need to make `processedIds` final - or you could change it, and since you don't `synchronized` nor `volatile`, there is no guarantee that threads would see the change in time. – Erwin Bolwidt Jun 24 '19 at 11:25
  • Why should I make it `final`? Where will it help me? Could you please explain? @ErwinBolwidt – Mansur Jun 24 '19 at 11:30
  • Because if it's not `final`, you can change it, and you haven't protected that variable with `synchronized` or `volatile`. If you don't intend to change a field, you should make it `final` - especially code where you are concerned about multi-threaded race conditions. – Erwin Bolwidt Jun 24 '19 at 11:31
  • No, I got the `synchonized` and `volatile` part. But doesn't final mean that I cannot re-assign the variable, but can change the internal state of it? I mean adding, removing, etc. – Mansur Jun 24 '19 at 11:32
  • Yes, but you just told us that the internal state of the object is thread-safe (it's a ConcurrentSkipListSet), so it's not a problem that you can change the internal state. – Erwin Bolwidt Jun 24 '19 at 12:21
  • Oh, I got it. Thanks for the explanation. I will make methods `synchronized` and see what happens. – Mansur Jun 24 '19 at 12:26

1 Answers1

1

I think that you deserve some clarification on the comments and also some clarification on what causes a race condition.

On race conditions-- the basic idea of all threading is that at anytime of execution the current thread that you are on could be rescheduled to a later time or another thread might be executing and accessing the same data in parrallel.

For one, the process IDs should be final as mentioned before. YOUR CODE WILL NOT BE THREAD SAFE UNTIL YOU DO THIS. Even though the ConcurrentSkipListSet<> is thread safe this doesn't stop the variable processIds from being reassigned by another thread. Also, Java is weird and to your processIds field must be marked final to guarantee that is initialized before the constructor completes. I found this stackoverflow post that explains some of the issues with object construction in java for some more reading. Constructor synchronization in Java. Basically, don't mark your constructor fields as synchronized, but if you want to guarantee variable initialization in the constructor, which in this case you do, then mark your field as final.

To answer your question on whether this is thread safe, the only answer would be that it depends on the client usage you are expecting. The methods you provided are indeed thread safe for their intended purposes as written, but a client could use them and produce a race condition. Your intuition on whether or not it would be 100% necessary to use the synchronized keyword is correct. Yet, what the comments are alluding too is that not making these methods explicitly thread safe might have some dire consequence in the future for the maintainability and correctness of your code.

A client could still use the API you provide in an unsafe way that could result in a race condition as mentioned in one of the comments. If you are providing an interface to a client you may not care about this... or you might care about this and want to provide a mechanism for the client to make multiple accesses to your class with guaranteed thread safety.

Overall, I would probably recommend that you mark your methods as synchronized for a couple of reasons 1) it makes in clear to the client that they are accessing methods that are thread safe which is very important. I can easily imagine a situation where the client decides to use a lock when it isn't needed at the detriment of performance. 2) Someone could change your methods to contain some different logic in the future that requires the synchronized keyword (this isn't so unlikely because you seem to already be in a threaded environment).

Sam Furlong
  • 106
  • 1
  • 6