ModeShape/modeshape — pull request #273 by rhauch (2012-01-18 19:18:44+00:00)
Title: MODE-1374 Corrected node type initialization errors in concurrent environment

The JcrNodeDefinition is intended to be immutable and thread-safe, and although it is publicly immutable, internally it delays loading the references to the required primary types until needed. The `ensureRequiredPrimaryTypesLoaded()` method is what lazily initializes the references, and this method is properly called in the right spots within JcrNodeDefinition. However, in a highly-concurrent environment a node type definition may be needed by multiple threads, and this can result in this method being called concurrently. Unfortunately this method is not thread-safe, and can cause a NullPointerException when used concurrently.

The `ensureRequiredPrimaryTypesLoaded()` method just iterates through the names of the required primary types (which were set in the constructor), and looks up the JcrNodeType instances for each primary type name. It then creates an immutable map to quickly find the JcrNodeType instances given the primary type name. Finally, it stores the map and the names, and when the array of JcrNodeType instances and map are available (non-null), it merely uses the values. But the creation of the JcrNodeType array and map keyed by name are not concurrent or atomic. In fact, a non-empty array is created (but not initialized, so it contains null references) and _immediately set_ on one of the JcrNodeDefinition fields, so when the method is called by other threads it is as if the JcrNodeType array is ready to be used. But the original thread hasn't yet set the JcrNodeType references in the array, so any other thread that uses the array gets a null reference instead, and using that causes the NullPointerException.

The fix is relatively simple: ensure that the `requiredPrimaryTypes` array of JcrNodeType and `requiredPrimaryTypesByName` map are set in a synchronized way, ensuring they are only set to non-null with an array (or map) that are fully and correctly populated. This could be done by simply synchronizing the entire `ensureRequiredPrimaryTypesLoaded()` method, but the method calls out to the RepositoryNodeTypeManager. And while the RNTM doesn't currently (directly or indirectly) result in a call to `ensureRequiredPrimaryTypesLoaded()`, it's possible it might in the future. Therefore, we can simply create and fully-populate the array and map outside of the synchronization block, and only synchronize upon setting
it (of course checking before we set it whether another thread snuck in and set it while we waited for the lock). So it is effectively the same as synchronizing the method (which the reporter says fixes the problem), but does it with smaller lock scope.

With this change, all unit and integration tests pass.