[Pki-devel] [PATCH] 0134 Block reads during reload of LDAP-based profiles

[Endi Sukma Dewata]

On 9/14/2016 7:14 AM, Fraser Tweedale wrote:
> Hi team,
>
> The attached patch fixes (yet another) race condition in
> LDAPProfileSubsystem.
>
> https://fedorahosted.org/pki/ticket/2453
>
> Additional context: https://fedorahosted.org/freeipa/ticket/6274
>
> Thanks,
> Fraser

The patch looks fine, but probably it can be simplified like this:

class LDAPProfileSubsystem {

     void init() {

         // load initial profiles
         repository = new LDAPProfileRepository();
         repository.load();

         // monitor profile changes in the background
         monitor = new Thread(repository);
         monitor.start();
     }

     IProfile getProfile(id) {
         return repository.getProfile(id);
     }
}

class LDAPProfileRepository {

     LinkedHashMap profiles = ...

     void synchronized load() {

         // create persistent search
         conn = dbFactory.getConn();
         results = conn.search(...);

         // get number of profiles
         entry = results.next();
         numProfiles = entry.getAttribute("numSubordinates");

         for (i=0; i<numProfiles; i++) {
             // read profile
             entry = results.next();
             readProfile(entry);
         }
     }

     void synchronized readProfile() {
         ...
     }

     IProfile synchronized getProfile(id) {
         return profiles.get(id);
     }

     void run() {

         while (true) {
             try {
                 // process profile changes
                 while (results.hasMoreElements()) {
                     entry = results.next();
                     ...
                 }
             } catch (...) {
                 // reconnect
                 load();
             }
         }
     }
}

So the load() will block during initialization and will also block 
readers during reload after reconnect. We probably can replace 
"synchronized" with ReadWriteLock to allow concurrent readers.

Feel free to push the patch as is (assuming it's well tested). We can 
make further improvements later on.

One thing though, I highly suggest that we fix this issue on both Fedora 
and RHEL/CentOS platforms. The patch is non-trivial, so the behavior 
could be different if not applied consistently. Since PKI is developed 
mainly on Fedora but used on different platforms, it would be much 
easier to troubleshoot issues by keeping the behavior consistent across 
platforms, especially on anything related to concurrency.

We don't need to create new builds for all platforms at the same time, 
but we should at least push this patch to all 10.3 branches so it can be 
picked up in the next 10.3 build of the corresponding platform.

-- 
Endi S. Dewata