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

[Fraser Tweedale]

On Thu, Sep 15, 2016 at 03:39:39PM +1000, Fraser Tweedale wrote:
> On Wed, Sep 14, 2016 at 07:16:32PM -0500, Endi Sukma Dewata wrote:
> > 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.
> > 
> Yep, that's a good approach.
> 
> > 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.
> > 
> The patch is (at this stage) not destined for 10.3 at all.  I'd
> prefer to push it to master to be included in Fedora when 10.4 gets
> released, and other platforms' builds whenever they rebase.
> 
> I might go ahead and implement your suggested change before merging,
> too, although probably as a second patch.
> 
On further investigation, the suggested approach will require either
moving a lot of logic out of the AbstractProfileSubsystem base class
or signficiant rework to make all profile subsystem implementations
(LDAP and File) use a 'ProfileRepository' concept.

Might be a good refactoring candidate for future.

Original patch pushed to master
(ced5cb71c1963d5234c2360d1f2ac11d4a452d9d)

Cheers,
Fraser