ConcurrentModificationException when executing AutowireCapableBeanFactory.createBean [SPR-13493] #18071
Comments
|
Juergen Hoeller commented Any indication where the update to that bean definition names list is coming from? The bean definition names should be stable after the bean registration phase, so I'm surprised that any updates come in here. Also, the stacktrace indicates a Juergen |
|
Richard Costine commented Yes, my bad... from the stack trace the underlying object being iterated is a LinkedHashMap, not a List (sorry, long week). Still, same difference - you can't get an iterator from the object, then do a put (or remove) to the LinkedHashMap, then do a next on the iterator. It'll fail. We're still trying to figure out how beans are being (presumably) added to the context during the time a createBean is getting executed. We are in an OSGI environment, and using the 4.1.6 apache servicemix bundles. Gemini Blueprint is being used. As you can see from the stack trace, Akka is in the mix. Multiple contexts are getting started at the same time. The whole thing is kind of hard to follow. We were kind of looking for an easy way to switch on the old behavior and somehow avoid a lot of concurrency-related code rework, which could possibly introduce other problems. We will continue to work on our side to try to figure out what might be dynamically configuring the underlying bean factory after it is supposed to be stable. Thanks, |
|
Juergen Hoeller commented There seems to be a mismatch between your stacktrace and the line numbers in the actual 4.1.6 release: Juergen |
|
Richard Costine commented Short answer, the implied iterator is ultimately gotten from the underlying backing Map of the LinkedHashSet, which is a LinkedHashMap. Long answer: Here is the field in DefaultListableBeanFactory:471 that is being iterated over: /** List of names of manually registered singletons, in registration order */
private final Set<String> manualSingletonNames = new LinkedHashSet<String>(16);A LinkedHashSet is implemented using a LinkedHashMap under the covers. The for loop at line 471: // Check manually registered singletons too.
for (String beanName : this.manualSingletonNames) {
}is really syntactic sugar for something like: final Iterator<String> it = this.manualSingletonNames.iterator();
while(it.hasNext()) {
String beanName = it.next()
....
}The LinkedHashSet constructor calls the special super HashSet(...) constructor. It looks like this (we're using JDK 1.8): /**
* Constructs a new, empty linked hash set. (This package private
* constructor is only used by LinkedHashSet.) The backing
* HashMap instance is a LinkedHashMap with the specified initial
* capacity and the specified load factor.
*
* @param initialCapacity the initial capacity of the hash map
* @param loadFactor the load factor of the hash map
* @param dummy ignored (distinguishes this
* constructor from other int, float constructor.)
* @throws IllegalArgumentException if the initial capacity is less
* than zero, or if the load factor is nonpositive
*/
HashSet(int initialCapacity, float loadFactor, boolean dummy) {
map = new LinkedHashMap<>(initialCapacity, loadFactor);
}There's where the LinkedHashMap is created. Since the iterator is off of that, you get the stack trace as shown above. |
|
Richard Costine commented LinkedHashSet public constructor that calls the above private HashSet(...) constructor specifically used by LinkedHashSet. /**
* Constructs a new, empty linked hash set with the specified initial
* capacity and the default load factor (0.75).
*
* @param initialCapacity the initial capacity of the LinkedHashSet
* @throws IllegalArgumentException if the initial capacity is less
* than zero
*/
public LinkedHashSet(int initialCapacity) {
super(initialCapacity, .75f, true);
} |
|
Richard Costine commented Also, in some cases we've also seen ConcurrentModificationException(s) around line 423 as well: // Check all bean definitions.
for (String beanName : this.beanDefinitionNames) {Where beanDefinitionNames is a List: /** List of bean definition names, in registration order */
private final List<String> beanDefinitionNames = new ArrayList<String>(64);I think that may have been part of the confusion in the original description (where I mentioned that the ConcurrentModificationException was happening in a List iteration vs a Set iteration)... since I remember also seeing exceptions at that line (423), and this.beanDefinitionNames is a List. Again, my bad for not looking closer at the above exception before I posted. |
|
Juergen Hoeller commented Indeed, thanks for pointing this out! I even looked at The key question remains: Where does the concurrent registration of bean definitions (or singleton bean instances not backed by a bean definition) come from here? Or, from another perspective, why are bean retrieval calls coming in concurrently while bean factory bootstrap - i.e. the Eventually, we may have to make that code more defensive again... possibly using a Juergen |
|
Juergen Hoeller commented Taking a step back, the problem is not really in the data structures but in the attempt to do such parallel registration and retrieval with autowiring to begin with: The autowiring results are not going to be stable, with some attempts succeeding or failing just because of their timing. A On a related note, before that efficiency-motivated change in the 4.1 line, we were simply always iterating over array copies of those underlying collections. Even back then, the name-listing collections were not concurrent either: just the same regular order-preserving data structures that we have now. A I assume that your specific scenario might be manageable in that respect. I'm just not sure it's a good idea to generally support such concurrent registration scenarios out of the box, since it arguably isn't ever as reliable as you'd expect it to be. That said, if it is easy enough to mitigate, we'll try to do something about it... maybe as a mode of operation. Juergen |
|
Juergen Hoeller commented After a bit of experimentation, there seems to be a sweet spot: We can dynamically switch to a Concretely, we bootstrap with long-lived This arrangement keeps iteration stable since we never concurrently modify an existing collection, and common startup registration scenarios remain very efficient: We're talking about factors between 50x and 1000x times faster compared to the use of I'm committing this for 4.2.2 now, letting it go through the Spring Boot 1.3 RC phase. However, with the extent of this change and potential subtle implications, I won't backport it to 4.1.8. Juergen |
|
Richard Costine commented Thanks, Juergen. We'll be looking forward to the new changes in the official 4.2.2 release coming in October. Meanwhile we will probably be pulling down the latest 4.2.2 and building from source in order to test your changes. Of course, since we need to have OSGI-bundlefied jars (this project still uses OSGI, and needs the manifest info in the jar), we'll also have to create them as well, since the apache servicemix bundles don't exist for this release (yet). |
|
Juergen Hoeller commented Richard, we're working towards the 4.2.2 release next Wednesday, September 30th, so it's not far away. If you have any chance to test a snapshot upfront, that would of course be great... Juergen |
|
Richard Costine commented Thanks again for the quick turnaround on this. We might have a build of our application with the new Spring version changes ready as soon as monday afternoon. |
|
Archie Cobbs commented I encountered this problem using Spring 4.1.x. Glad to know it's now "fixed" so to speak. However, there still remains the question: what is the correct way, in a Spring bean, to say "block until the context containing this bean has finished initializing", which is the only truly airtight way to fix this problem? In my case I had code looking like this: public void afterPropertiesSet() throws Exception {
this.taskScheduler.scheduleWithFixedDelay(new Runnable() {
public void run() {
MyBean.this.doSomething();
}
}, DELAY); // note: first execution is immediate
}
private void doSomething() {
new AutowiredBean() ... // exception occurs in AutowiredBean constructor
}What's needed is something like this: private void doSomething() {
this.beanFactory.waitForInitialization();
new AutowiredBean() ... // this is now guaranteed to succeed
}What is the officially correct way to handle this ? |
|
Archie Cobbs commented Nevermind previous question.. I realize now that use of |
spring-projects-issues
added
type: bug
Richard Costine opened SPR-13493 and commented
This error is now happening sporadically during our acceptance tests. It looks to be caused by an internal list of strings (in spring's DefaultListableBeanFactory) which is updated via another thread, while an Iterator (backed by that same list of strings) is cycling over it.
If something is added (or deleted) to (or from) a collection while the collection is being iterated over, a ConcurrentModificationException will occur inside the next().
We have recently upgraded from Spring 3.1 to 4.1.6. With both Spring releases, we are using Akka and running in an OSGI container. When actors are instantiated, a spring createBean is done. That is where this sporadic exception now occurs.
There is a similar issue here:
#17286
The ConcurrentModificationException issue didn't occur with 3.1 because a copy of the underlying list was Iterated over, not the original list. It looks like this was changed for performance reasons, but now it is causing our application to fail sporadically. I looked at code for the 4.2.1 release and the lists are still not accessed in a threadsafe manner.
If there are no plans to fix this code to ensure that the lists are accessed in a synchronized fashion (using a CopyOnWriteArrayList, or by giving the option for the lists to be copied before attempting to Iterate over them, or perhaps synchronizing on the lists around the iteration) is there any way for us to tell the context to use our own custom bean factory that does this? We are willing to sacrifice some performance degradation, in order to allow the application to still function similar to the way it was working under the old 3.1 release.
Currently we are synchronizing around the createBean, and for some cases it seems to fix the problem. However, this is a very large application and we feel the best way to ensure that it is fixed everywhere is to ensure that the beanFactory itself can handle access by multiple threads.
Affects: 4.1.6
Issue Links:
Referenced from: commits 097bcfb
1 votes, 4 watchers
The text was updated successfully, but these errors were encountered: