Code Review · Java

Java Code Review: Thread Safety Assumptions

Review Java service code that uses a shared HashMap cache without considering concurrent requests.

Code Review Exercise

Review the following service code. Assume this service is used by a web application where multiple requests may call it at the same time.

java

public class UserService {
    private final Map<String, User> cache = new HashMap<>();
    private final UserRepository repository;

    public UserService(UserRepository repository) {
        this.repository = repository;
    }

    public User getUser(String id) {
        if (cache.containsKey(id)) {
            return cache.get(id);
        }

        User user = repository.findById(id);
        cache.put(id, user);
        return user;
    }
}

What concerns do you see before scrolling further?

Review Findings

Issue #1: HashMap is not thread-safe

HashMap is not safe for concurrent reads and writes. If multiple requests call getUser() at the same time, the shared cache can behave incorrectly.

Issue #2: Check-then-act race condition

The code first checks containsKey(), then later calls get() or put(). Another thread can modify the map between those operations.

Issue #3: Duplicate database calls

Two threads asking for the same missing user may both call the repository before either one updates the cache. That can cause unnecessary database load.

Issue #4: Unbounded cache growth

The cache never evicts anything. Even if thread safety is fixed, this service can keep accumulating users forever and eventually create memory pressure.

Why This Happens

A common mistake is assuming that code is safe because each method looks simple in isolation.

In a web application, service objects are often shared. That means a field like cache may be accessed by many threads at the same time.

The review question is not only whether the code works for one request. It is whether it still works when many requests hit it concurrently.

Improved Version: ConcurrentHashMap

java

public class UserService {
    private final Map<String, User> cache = new ConcurrentHashMap<>();
    private final UserRepository repository;

    public UserService(UserRepository repository) {
        this.repository = repository;
    }

    public User getUser(String id) {
        return cache.computeIfAbsent(id, repository::findById);
    }
}

ConcurrentHashMap makes concurrent access safer, and computeIfAbsent() expresses the cache lookup pattern more clearly.

But This Is Still Not a Complete Cache

The improved version is safer, but it still has product and operational questions:

Should cached users expire?
How large can the cache grow?
What happens if the database value changes?
Should missing users be cached?
Should this use a real caching library instead?

In production code, a library such as Caffeine or an external cache such as Redis may be more appropriate depending on the use case.

Interview Follow-Ups

Is HashMap safe if there are only reads?

Concurrent reads are generally not the issue. The danger appears when at least one thread may modify the map while others are reading or writing.

Does ConcurrentHashMap fix every concurrency problem?

No. It makes individual map operations thread-safe, but the overall design may still have race conditions, stale data, or cache invalidation problems.

Why is containsKey() followed by get() a smell here?

It splits one logical operation into multiple steps. In concurrent code, another thread can change the state between those steps.

When should this use a real cache instead?

When you need eviction, expiration, size limits, metrics, refresh behavior, or distributed caching. A plain map is usually too simple for those requirements.

What Interviewers Expect

A basic review might only say, HashMap is not thread-safe.

A stronger review connects several concerns:

Correctness: shared mutable state is accessed concurrently.
Performance: duplicate repository calls are possible.
Maintainability: the cache behavior is implicit.
Operations: there is no eviction, expiration, or size limit.
Design: a real cache abstraction may be better than a raw map.