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