[Pki-devel] [PATCH] 0057..0061 LDAPProfileSubsystem concurrency fixes

[Endi Sukma Dewata]

On 11/30/2015 10:56 PM, Fraser Tweedale wrote:
> The attached patches resolve concurrency issues in the
> LDAPProfileSubsystem (which conducts its LDAP persisent search in
> background thread).
>
> Ticket: https://fedorahosted.org/pki/ticket/1700
>
> Thanks,
> Fraser

Patch #57 is ACKed.

Patch #58 is ACKed with some comments:

1. To help troubleshooting, the static initializer in 
LDAPPostReadControl should log the exception:

     static {
         try {
             register(OID_POSTREAD, LDAPPostReadControl.class);
         } catch (Throwable e) {
             e.printStackTrace();
         }
     }

2. In LDAPPostReadControl constructor is it necessary to catch & ignore 
the exception? I think the String constructor doesn't declare any 
throwable exceptions. If necessary, we could wrap the original exception 
and rethrow it so we'll know if there's an error.

     try {
         dn = new String(buf, "UTF8");
     } catch(Throwable e) {
         throw IOException(e);
     }

3. The LDAPPostReadControl is used for both request and response. It's 
not an issue, but it would be nice to use separate classes since they 
have different contents (e.g. the request doesn't have an LDAP entry).

Patch #59 is ACKed with some comments:

1. In AbstractProfileSubsystem and LDAPProfileSubsystem, the 
commitProfile() should chain the original exception to help troubleshooting:

      throw new EProfileException(
          "Failed to commit config store of profile '" + id + ": " + e,
          e);

2. In LDAPProfileSubsystem it would be nice to have a time- or 
size-based cache expiration for the entryUSNs map, but assuming the 
number of profiles will stay relatively small this may not be a problem. 
Just something to keep in mind.

More will follow.

-- 
Endi S. Dewata