Spring singleton beans, maps and multithreading

Question

Sometimes I used to do things like this:

@Component class MyBean {
  private Map<TypeKey, Processor> processors;

  @Autowired void setProcessors(List<Processor> processors) {
    this.processors = processors.stream().....toMap(Processor::getTypeKey, x -> x);
  }

  //some more methods reading this.processors
}

But, strictly speaking, it's buggy code, isn't it?

1) this.processors is not final, nor is its creation synchronized on the same monitor as every access to it. Thus, every thread - and this singleton can be called from arbitrary thread processing user request - may be observing its own value of this.processors which might be null.

2) Even though no writes happen after the Map is initially populated, Javadoc offers no guarantees on which implementation will be used for the Map, so it might be an implementation not ensuring thread safety when the Map structure changes, or even if anything is modified, or at all. And initial population is changes, so it may break thread safety for who knows how long. Collectors even offer the specialized toConcurrentMap() method, to address that problem - so, at a bare minimum, I should have been using it instead.

But even if I use toConcurrentMap() in #2, I will not be able to make my field final, because then I'll not be able to initialize it in a setter. So here are my choices:

a) Initialize and populate the Map in an autowired constructor, which frankly I prefer. But so few teams do that, so what if we abstain from that solution? What other choices exist?

b) Initialize the Map to an empty final ConcurrentHashMap, then populate it in a setter. This is possible, but we'll have to list.forEach() then map.put(). This looks like it's still Java 6; or we could definitely do map.addAll(list....toMap()) but its useless duplication of the Map, even if temporary.

c) Use volatile on the field. Slightly degrades performance without any need, because after some point the field never gets changed.

d) Use synchronized to access the field and read its values. Clearly even worse than (c).

Also, any of those methods will make the reader think that the code actually wants some multithreading reads/writes to the Map, while actually, it's just multithreaded reading.

So, what does a reasonable guru do when they want something like that?

At this point, the best solution seems to be the one with a volatile field, assigned in a setter by using toConcurrentMap. Is there anything better? Or maybe I am just making up problems no one ever actually encountered?

Answer 1

Or maybe I am just making up problems no one ever actually encountered?

I think may be conflating your assignment with the problems historically seen from double-checked locking:

private Foo foo;  // this is an instance variable

public Foo getFoo() {
    if (foo != null) {
        synchronized (this) {
            if (foo != null) {
                foo = new Foo();
            }
        }
    }
    return foo;
}

This code appears to be thread-safe: you do an initial, presumed quick, check to verify that the value has not been initialized yet, and if it hasn't you initialize in a synchronized block. The problem is that the new operation is distinct from the constructor call, and some implementations were assigning the reference returned by new to the variable before the constructor ran. The result was that another thread could see that value before the constructor completed.

In your case, however, you are assigning the variable based on the result of a function call. The Map created by the function call is not assigned to the variable until the function returns. The compiler (including Hotspot) is not permitted to re-order this operation because such a change would be visible to the thread that's executing the function, and would not therefore be sequentially consistent per JLS 17.4.3.

That out of the way, here are some additional comments:

Initialize and populate the Map in an autowired constructor, which frankly I prefer

As do the creators of the Guice dependency injection framework. One reason to prefer constructor injection is that you know that you'll never see the bean in an inconsistent state.

The reason that Spring encourages (or at least does not discourage) setter injection is because it makes circular references possible. You can decide for yourself whether circular references are a good idea.

Initialize the Map to an empty final ConcurrentHashMap, then populate it in a setter

This is a bad idea because it's likely that other threads will see a partially constructed map. It's far better to see either null or a fully-constructed map, because you can compensate for the first case.

Use volatile on the field. Slightly degrades performance without any need

Use synchronized to access the field and read its values. Clearly even worse than (c).

Don't let perceived performance impacts keep you from writing correct code. The only time that synchronization will significantly impact your performance is when concurrent threads access a synchronized variable/method within a tight loop. If you're not in a loop, then the memory barrier adds an irrelevant amount of time to your call (and even in a loop it's minimal unless you need to wait for a value to arrive in your core's cache).

In this case it doesn't matter, but I would guess that getProcessors() takes a tiny percentage of your total execution time, and that a far larger amount of time is taken by running the processor(s).

Answer 2

Thanks to hints of commenters here, after googling a bit I was able to find not a reference to Spring manual but at least a matter-of-fact guarantee, see Should I mark object attributes as volatile if I init them in @PostConstruct in Spring Framework? - the updated part of accepted answer. In essence it says that every lookup of a particular bean in a context, and hence, roughly, every injection of that bean, is preceded by locking on some monitor, and bean initialization also happens while locked on that same monitor, establishing happens-before relationship between bean initialization and bean injection. More specifially, everything done during bean initialization (like, assignment of processors in MyBean initialization) happens-before subsequent injections of that bean - and the bean is only used after it has been injected. So the author says no volatile is necessary, unless we are going to change the field after that.

That would be my accepted answer (in combination with toConcurrentMap) if not for 2 "buts".

1) This does not mean injection of non-initialized beans offers that same happens-before. And injection of non-initialized beans happens more often that some think. In case of circular dependencied, which are better kept rare but look valid sometimes. In case of lazy initialized beans. Some libraries (AFAIK even some of Spring projects) introduce circular dependencies, I saw that myself. And sometimes you introduce circular deps by accident, that is not treated as error by default. Of course, reasonable code does not use non-initialized beans, but since MyBean could be injected to some bean X before it's initialized, there will be no happens-before after it's initalized, annihilating our guarantees.

2) This is not even a documented feature. Still! But recently it has at least been put on a backlog. See https://github.com/spring-projects/spring-framework/issues/8986 - still, until they documented it we cannot assume it's not subject to changes. Bah, even when they do, it still may be changed in some next version, but at least that will be reflected in some changelist or whatnot.

So, taking those 2 notes into consideration, especially the 1st, I am inclined to say that volatile+toConcurrentMap is the way to go. Right?