Threading issue in InjectionMetadata.InjectedElement [SPR-9263]

[spring-projects-issues]

Anders Kobberup opened SPR-9263 and commented

We are having a rising amount of issues where the symptom is that random properties that have been autowired into our beans (via @autowired setter methods) turns our null, without any exceptions during start up.
This means that we have no way of knowing whether our production setup is actually functioning as expected, or if it will encounter nullpointers as the diferent parts of the platform is used. Also the only way to "fix" any such nullpointer is to restart the application, which obviously is not good.

I have traced the problem down to what i believe is a concurrency flaw in checkPropertySkipping in InjectionMetadata.InjectedElement.

172: protected boolean checkPropertySkipping(PropertyValues pvs) {
173: 	if (this.skip == null) {
174: 		if (pvs != null) {
175: 			synchronized (pvs) {
176: 				if (this.skip == null) {
177: 					if (this.pd != null) {
178: 						if (pvs.contains(this.pd.getName())) {
179: 							// Explicit value provided as part of the bean definition.
180: 							this.skip = true;
181: 							return true;
182: 						}
183: 						else if (pvs instanceof MutablePropertyValues) {
184: 							((MutablePropertyValues) pvs).registerProcessedProperty(this.pd.getName());
185: 						}
186: 					}
187: 				}
188: 			}
189: 		}
190: 		this.skip = false;
191: 	}
192: 	return this.skip;
193: }

Consider the scenario where at least two threads are creating an instance of a given bean, which have autowired properties. Both of these threads will enter the 'Inject(...)' method, and from here go to checkPropertySkipping (the argument for both threads are the same PropertyValues object).

Thread A will enter the synchronized block, while Thread B will be halted before this block.
Thread A will proceed to the inside of the synchronized block, and enter the 'else if' block, and register the propertyname in the PropertyValues object as processed (line 184).
As soon as Thread A exits the synchronized block, the java scheduler halts Thread A and resumes Thread B which will enter the synch block and, as Thread A has been halted and thus not having set 'this.skip = false' (line 190), the 'if(this.skip == null)' statement (line 176) will return true.
After this, thread A may continue running (the object that it is creating will be created correctly).

Thread B will evaluate the 'if (pvs.contains(this.pd.getName()))' statement, which will return true, as Thread A has registered the property name as processed, and as a result Thread B will set 'this.skip = true' (line 180) and return.

As 'skip' has been set to true on this InjectedElement, every later invocation of checkPropertySkipping after this will return true, and thus every later object that is instantiated will not have this dependency injected.

The obvious solution is to ensure that 'this.skip = false' is also set within the synchronized block (add a line containing 'this.skip = false;' after line 187), thus ensuring that any threads that are awaiting the monitor will evaluate the if statement at line 176, to false and thus not setting 'this.skip = true'.

In order to see if this is more than just a theoretical posibility that multiple threads will enter this code, i have created a small test class that can simulate this method being invoked by multiple threads. The test i have run, show that it will happen.

import java.util.ArrayList;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;

public class SynchTest {

	private final Object mutex = new Object();
	private Boolean finished;
	private AtomicInteger numberOfCompletions = new AtomicInteger( 0 );
	private int testNumber;

	public SynchTest( int TestNumber ) {
		this.testNumber = TestNumber;
	}

	public Runn getRunn() {
		return new Runn();
	}

	void gotThrough() {
		final int nCount = numberOfCompletions.incrementAndGet();
		if ( nCount > 1 ) {
			//woops - this is not good!!
			System.out.println( "In test " + testNumber + ", " + nCount + " got into the synch block!!" );
		}
	}

	private class Runn implements Callable<Object> {

		@Override
		public Object call() throws Exception {
			if ( finished == null ) {
					synchronized ( mutex ) {
						if ( finished == null ) {
							gotThrough();
						}
					}
				finished = false;
			}
			return null;
		}
	}

	public static void main( String[] args ) throws InterruptedException {
		ExecutorService cThreadPool = Executors.newFixedThreadPool( 5 );
		ArrayList<Runn> cRunnables = new ArrayList<>();
		for ( int i = 0 ; i < 1000 ; i++ ) {
			SynchTest cTest = new SynchTest( i );
			for ( int j = 0 ; j < 5 ; j++ ) {
				cRunnables.add( cTest.getRunn() );
			}

		}
		System.out.println( "Invoking "+cRunnables.size()+" tasks" );
		cThreadPool.invokeAll( cRunnables );
		cThreadPool.shutdown();
		cThreadPool.awaitTermination( 5, TimeUnit.DAYS );
	}
}

Heres the result of just one run:

Invoking 5000 tasks
In test 695, 2 got into the synch block!!
In test 765, 2 got into the synch block!!
In test 824, 2 got into the synch block!!
In test 872, 2 got into the synch block!!
In test 981, 2 got into the synch block!!

---

Affects: 3.1.1

Issue Links:

• Autowired properties can remain unset during concurrent instantiation of prototype-beans [SPR-9806] #14439 Autowired properties can remain unset during concurrent instantiation of prototype-beans ("duplicates")
• Concurrent retrieval of prototype-scoped beans may result in null @Autowired fields [SPR-9256] #13894 Concurrent retrieval of prototype-scoped beans may result in null @Autowired fields

[spring-projects-issues]

Chris Beams commented

Thanks for the test case, Anders. We'll take a look.