As a Baeldung editor, I had the pleasure working with an author on an article on Common Concurrency Pitfalls in Java. This is a great read, but assumes a certain amount of competence on the part of the developer.
There are a couple of things I’ve seen which are instant concurrency fails. They’re easy to add to the code, and guaranteed to give you weird results. The fact that developers still commit these is a comment on how we educate them about OO and concurrency, which are very dangerous when used incorrectly.
A Code Review Aside
As a code reviewer, I’ve developed a few shorthands over the years. These help me spot areas to look at in more detailed in a big code change. They include red flags things I expect to go wrong. It’s a good idea to train oneself to spot key anti-patterns, or potential anti-patterns, as they can be valid code, but result in invalid behaviour.
Request State in Bean
In a Java application, the services, controllers, handlers and repositories are generally singleton. They’re created as the app is started, and then requests pass through them, often from multiple threads.
Consider code like this:
In this, the author of the class has decided that the object can remember the item it’s presently working on, to save effort in terms of passing that item down to the next function.
This violates two principles: thread-safety and meaningful object state. It’s very unlikely that an order processor is really about the order it’s processing. You might imagine something that statefully iterates through an order, some sort of cursor, reader, or builder, but blending all of those together in a single object is muddy.
Most importantly, though, there’s a clear definition of why this is wrong. If every the attribtues of a request get put into the receiver of that request, then you have two risks:
- Bleed between requests in a multi-threaded execution
- Bleed between requests in single-threaded if things aren’t fully tidied up
In short, never do it!
Crazy Lazy Initialization
Lazy initialization allows for:
- Faster startup because of
- Just-in-time loading of resources when needed
- No loading of a resource if it’s not needed (example, a serverless Lambda which may never be asked to do a certain code path in its lifetime)
- Customisation of how a resource is loaded by activities that happen sooner
All of these are good. However, this code:
Though it will work, it can be called concurrently and will go wrong. How wrong it goes, depends on all sorts of things. In the example, I’ve tried to hint at the sorts of things we’re dealing with:
- In concurrent calling, more than one lazy-load happens…
- … if this is expensive, it’s a waste
- If more than one lazy-load happens, perhaps two objects stay resident in memory for longer than needed, or forever
- If this is meant to be a singleton, perhaps the request that gets the orphaned object somehow fails to coordinate with the rest of the requests
- Using a hand-made non-thread-safe object initialization is a real pity
For correct singleton initialization you should use double-checked locking or use a framework, or even judicious use of simple Java singletons based around
Other Concurrency Fails
The above two seem the most common things to get so wrong that it should be obvious. If you spot another, then please drop it in the comments.