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

[Fraser Tweedale]

Hi Endi, thanks for reviewing.  I've addressed your comments
(commentary inline) and pushed 57..59 to master:

0057 d272cec2614a4a45abd3fdbf7139dbd52b3275ae
0058 2bd89f148b4b347fc80285ec521d2af0299da746
0059 81af68d3e3b1a89f799693e7f7ecda59f57abfe4

On Tue, Jan 12, 2016 at 01:20:47PM -0600, Endi Sukma Dewata wrote:
> 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();
>         }
>     }
> 
Done.

> 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);
>     }
> 
This constructor can throw UnsupportedEncodingException which is a
subclass of IOException.  IOException is declared by the method so I
changed it to just let the exception through.

> 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).
> 
Not possible, unfortunately.  The same OID is used for both the
request and response structures, and the control is registered by
OID.

> 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);
> 
Done, thanks.

> 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.
> 
Size is linear in number of profiles; nothing to worry about :)

> More will follow.
> 
> -- 
> Endi S. Dewata
>
Cheers,
Fraser