[SSSD] [PATCH] Fix startup race condition causing backend restarts
Stephen Gallagher
sgallagh at redhat.com
Mon Aug 10 10:52:11 UTC 2009
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 08/10/2009 03:38 AM, Martin Nagy wrote:
> On Fri, 07 Aug 2009 13:59:49 -0400, Stephen Gallagher
> <sgallagh at redhat.com> wrote:
>> When the backends start up, the monitor was immediately sending a
>> getIdentity request. However, as we've added more processing to
>> the initialization routines over time, we started introducing
>> latency between when we open the connection and when we're able to
>> process requests on that connection.
>>
>> I've updated the monitor to check for NoReply as an error message
>> and queue the getIdentity check for retries until the service
>> answers or one wallclock second (literally, ten tries at 10ms
>> each) has passed.
>
> Hi Steven, the patch looks good to me. Ack.
> I do have some minor suggestions though, apply them at your own
> discretion.
>
>> static void identity_check(DBusPendingCall *pending, void *data)
>> @@ -1862,8 +1886,11 @@ static void identity_check(DBusPendingCall *pending, void *data)
>> DBusError dbus_error;
>> dbus_uint16_t svc_ver;
>> char *svc_name;
>> + const char *error_name;
>> dbus_bool_t ret;
>> int type;
>> + struct timeval tv;
>> + struct tevent_timer *te;
>
> I would put these two variables closer to where they are needed (marked
> below).
>
> [snip]
>
>> + error_name = dbus_message_get_error_name(reply);
>> + if (strcmp(error_name, DBUS_ERROR_NO_REPLY) == 0) {
>
> Put the 'tv' and 'te' variable declarations here.
>
I generally would, but the coding style within the SSSD prefers
declaration at the top of the function. If I have misunderstood our
coding style here, someone please tell me.
>> + /* Queue up another try in 10ms */
>> + fake_svc->restarts++;
>> + gettimeofday(&tv, NULL);
>> + tv.tv_usec += 1000;
>> + te = tevent_add_timer(fake_svc->mt_ctx->ev, fake_svc, tv,
>> + do_identity_check, fake_svc);
>> + if (te == NULL) {
>> + DEBUG(0, ("Failed to add do_identity_check event for [%s]!\n",
>> + fake_svc->name));
>> + sbus_disconnect(conn_ctx);
>> + return;
>> + }
>> + DEBUG(3, ("getIdentity reported NoReply. Retrying.\n"));
>
> Move this debug statement right at the beginning of the if() body.
>
> Martin
I put this at the end because it is only true if tevent_add_timer()
succeeded here. (Otherwise we are not retrying, we've just failed).
Thank you for the review.
- --
Stephen Gallagher
RHCE 804006346421761
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
iEYEARECAAYFAkp/+9YACgkQeiVVYja6o6PJKwCgizu18ymzsbeqPMmwiSWf9h5W
qzcAnRqIhjWzJTfoO7jwVr04HNS3MceQ
=4VfB
-----END PGP SIGNATURE-----
More information about the sssd-devel
mailing list