dispatch_init vs. dispatch_accept

[gperciva]

tl;dr: There's an API mismatch between lbs/dispatch.h and kvlds/dispatch.h (which has carried over to all the other programs).

lbs/ and mux/ have a dispatch_init(), which obviously allocates things; and dispatch_done(), which unconditionally cleans up and frees the dispatcher. That seems nice and clean to me.

By contrast, kvlds/ (and likely other programs) doesn't have a dispatch_init(); instead, dispatch_accept() allocates a new dispatcher. More problematically, dispatch_done() can only be called if dispatch_alive(D) has previously returned zero. In particular, it checks that (D->dying == 1), which only happens if it has called dropconnection().

1. are you sure that you want to allocate & free the dispatch object all the time in kvlds? Of course we need to ensure that each connection starts from a known state, but surely that could be restricted to dispatch_accept(), while a new dispatch_init() handles parameters that don't change, such as kmax and vmax.

2. I'm sure that kvlds needs a function to handle cleaning up after a dying connection (including sanity checks), but it's a bit confusing for that to have the same name as the "unconditional clean up" dispatch_done() from lbs. Could we rename one or both types of functions? My first thought would be rename dispatch_done() -> dispatch_zombie_cleanup() in dynamodb-kv, kvlds, lbs-dynamodb, lbs-s3, s3. (I'm not a huge fan of zombies; I'm just trying to be clear about the conditional nature of that clean-up.)

And then I'd like to add a dispatch_done() which acts the same as lbs dispatch_done(), namely unconditional clean-up.

On the other hand, given that there's only two programs which have unconditional clean-up, maybe it's better to leave the other programs alone, and rename the function in lbs and mux to dispatch_unconditional_done() or something like that.

[cperciva]

are you sure that you want to allocate & free the dispatch object all the time in kvlds?

Nope. I never particularly thought this through.

dispatch_zombie_cleanup

My intention was that there's only one connection at once, and you can't establish a new connection until the previous one is finished -- including any in-progress requests. So there shouldn't be "zombies" at any point.

(Obviously mux is the exception here since the whole point it exists is to allow multiple incoming connections to share access to a single daemon.)

I'm not sure if that clarifies things...

[gperciva]

Ah, perhaps "zombie" wasn't the right term. The point is that kvld's dispatch_done() insists that the dispatch object must be "dying":

/**
 * dispatch_done(D):
 * Clean up the dispatch state ${D}.  The function dispatch_alive(${D}) must
 * have previously returned zero.
 */
int
dispatch_done(struct dispatch_state * D)
{

        /*
         * There should not be a MR timer running, because there should be
         * no requests in progress.  We should not be reading a request, and
         * the connection should be dying.
         */
        assert(D->mr_timer == NULL);
        assert(D->nrequests == 0);
        assert(D->read_cookie == NULL);
        assert(D->dying == 1);

So if we've allocated a dispatch dstate but there's no connections, we're unable to clean it up.
(Why do we care about that? Well, we don't care about that particular thing, but I was looking into why FreeBSD ktrace says there's a memory leak, and noticed the discrepancy in the APIs)

I'll sketch out a dispatch_init() approach so that we can see what it might look like.

[cperciva]

At the point immediately after dispatch_accept returns, we can't free the dispatch state because it's waiting for a callback from network_accept.

We could record an accept_cookie and then cancel it in dispatch_done, I suppose. I guess I didn't see the point when I wrote that code... many many years ago.

[cperciva]

BTW I know that there's a lot of mostly-duplicated code between kivaloo daemons and I'd be happy to have it standardized and/or refactored (the first bit which comes to mind is callback_accept which is mostly identical between all the daemons). Not at all urgent but if you're interested in spending time cleaning things up...