Should services in a service layer be thread safe?

Question

This is for an Android app, but I think the question applies to any software designed with a service layer.

Our app is structured with a presentation layer that handles the UI and a service layer beneath it, comprising lots of service objects that the UI layer will call when it needs to perform some business logic. Say there's an EventRecordingService that records whenever the user clicks something and lots of UI classes hold a reference to the same EventRecordingService object.

Now lets say the EventRecordingService needs to assign an incrementing number to each event it sees, which means it has to maintain a counter. Obviously it would be better to be stateless but sometimes it can't be avoided. Now if an event is recorded from two different threads simultaneously, unless access to the counter is synchronized, it could get confused and give the wrong result.

My attitude to this kind of thing normally is to never make any classes thread safe unless they need to be, because thread safety is expensive and difficult. Right now, all calls to this service happen in the same thread and there's no plans to add more, so it's fine.

My colleague argues that it should be thread safe because in the future, someone else might come along and call the service from a different thread. The service doesn't look stateful from the outside, the counter is an implementation detail, and while there's no plans to add more calls to it currently it's easy to imagine it happening in the future. In this case not making it thread safe could be dangerous, because it might appear to work with the new call added but just occasionally go badly wrong.

I feel he has a point, but also, it seems like if we make everything thread safe even if it doesn't need to be we'll never get anything done.

So what's the normal thing to do in cases like this? Should we add a warning to the class saying it isn't thread safe, or make it thread safe? Or should we find a way to make it stateless at all costs?

Answer 1

Right now, all calls to this service happen in the same thread and there's no plans to add more, so it's fine.

This statement says it all. Right now the class satisfies the needs for which it was created. Thread safety is not a current requirement. YAGNI (Ya Ain't Gonna Need It) is the best guiding principle here. You are correct in saying thread safety is additional work, and right now it is extra work with no tangible benefit.

Only once you have a real world need for multiple threads calling this service class will you discover the true requirements for this level of concurrency. At that point the extra effort to make this class thread safe is worthwhile.

For the time being, KISS it — keep it simple. Write proper unit tests. Refactor later. Save time now.

Answer 2

I believe there's no "correct" answer to this question.
From my experience,

should we find a way to make it stateless at all costs

In many cases, such a class is impossible to imagine without a state, e.g. when it has some background queue, which you have to manage.

Should we add a warning to the class saying it isn't thread safe

Yes, you should add a warning to the interface, and some run-time logs if there're potential issues.

or make it thread safe

When speaking about multithreading, there're such perspectives as

1. a basic thread-safety ( shared data state is protected for all invariants )

2. fine-grained code: each thread holds some kind of lock for as small amount of time as possible.

Both 1 and 2 are relative - you can do evaluate an implementation as "bad", "good enough", "perfect" and so on.

As you mentioned that you don't have multithreading issues in your current codebase, but you may have them later, I'd advice to implement only "1", in "good enough" version.

And add a basic test coverage for your interface, so you know it works well in the simple cases, and have some control over regressions.

It's hard to make the implementation "perfect" from the first time, but in many cases you don't need it. And the basic multithreading support (mutex here, conditional lock there) isn't so expensive to add, and you can improve it later.

Answer 3

Say there's an EventRecordingService that records whenever the user clicks something and lots of UI classes hold a reference to the same EventRecordingService object.

Unless your UI is certainly special, typical UI frameworks are single-threaded for a number of reasons. Could it be possible that you don't really need a multi-threaded service because you are never going to actually use the service from multiple threads? Unless, of course, you intend to spawn threads from the UI, just for the sake of recording the event.

The service doesn't look stateful from the outside, the counter is an implementation detail

State cannot, and should not necessarily, be deducible from the "outside" (i.e. from the public API). If you do a good job with object-oriented services, only methods can be seen. Suggestions about what an API does should not be based on a "hunch" but on well-defined and clearly communicated meanings. The state of an implementer of an interface is not something that should be taken into account, it's the implementer's business. A consumer of an interface (the implementation of an interface) should be agnostic to whether the underlying implementation is stateless or not and should definitely not change their behavior based on whether it holds state or not.

Now if an event is recorded from two different threads simultaneously, unless access to the counter is synchronized, it could get confused and give the wrong result.

Thread-safety is expensive and complicated, indeed. But if this is your entire requirement, many frameworks provide facilities to make basic operations atomic. Interlocked.Increment is an example. Achieving thread-safety can be almost trivial in this case, so I suggest you find and use an atomic increment operation if your framework supports it (chances are it does).

I feel he has a point, but also, it seems like if we make everything thread safe even if it doesn't need to be we'll never get anything done.

Maybe he tries to make a point because of the triviality of making the underlying operation thread-safe. But bear in mind that the scenario you describe is extremely simple.

So what's the normal thing to do in cases like this?

If it is glaringly easy, as in incrementing a count variable, see if an atomic operation wrapper method is already provided by the framework, and use it, although chances are you are never going to use the service from multiple threads. Also, I suspect that if you are using an int, rather than a long (i.e. a 32-bit variable), you might not have a problem with atomicity anyway.

At the very least, make a small effort to make your class fail spectacularly when attempting to use it from multiple threads, so that you can remember to "upgrade" it when such a time comes. How? Well, a small step is to check for re-entrancy using a private bool variable, like:

//Check for re-entrancy.
if (_isWorking == true)
    throw new TheWorldIsFallingApartException("Ooops, I meant NotSupportedException! This class is not thread-safe yet.");

_isWorking = true;

//Do whatever you need to do here

_isWorking = false;

Of course, you can note that variable assignment is not always guaranteed to be an atomic operation but you shouldn't have a problem with booleans. So, unless I am wildly mistaken, such a small excerpt will at least protect you from obtaining weird results in the future because someone decided to go multi-threaded.