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 _______________________________________________ Pki-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/pki-devel
