Re: [PATCH] http: add_fill_function checks if function has been added

From: Tay Ray Chuan <rctay89@gmail.com>
Date: Mar 9, 2009, 12:01 UTC
Message-ID: <be6fef0d0903090501m21ee33b6pc3b45680324d932c@mail.gmail.com>
In-Reply-To: <7vd4crls8q.fsf@gitster.siamese.dyndns.org>

Hi,

On Mon, Mar 9, 2009 at 3:38 AM, Junio C Hamano <gitster@pobox.com> wrote:

> That is not what I was worried about.  There are two callsites to
> add_fill_function().

I'm sorry I didn't catch this from your earlier message; on a re-reading I caught it.

Show 8 quoted lines

> But it is not immediately obvious in http-walker.c callchain if multiple
> calls to add_fill_function(), if they were ever made, give the same
> "walker" as the callback data.  Your patch only looks at the function but
> not the callback data for its filtering.  Doesn't it mean that if a caller
> made two calls to add_fill_function() with walker#1 and then walker#2 as
> the callback data, you would discard the request for walker#2 and call the
> callback function fill_active_slot() with walker#1 inside run_active_slot
> repeatedly, without ever calling it for walker#2?

You're right, in addition to checking whether the callback is the same, I should also have checked the callback data, before discarding the invocation of add_fill_function as a duplicate.

Show 6 quoted lines

> If it is the former, then your change is introducing a bug. If it is the
> latter, your change won't break anything immediately, but still introduces
> a potential bug waiting to happen, as the API looks as if you can call
> add_fill_function() to ask for callback functions to be called with
> different callback data and the caller can rely on both of them to be
> called at appropriate times, but in reality that guarantee does not hold.

That indeed is true, but afaik, none of the two instances of the API usage does that -- calling callback functions with different callback data to change behaviour. The fill functions are basically used to "clear" the request queue.

Look at the fill function in http-walker.c. Yes, it's true that callback data is passed on by the fill function, unlike the one in http-push.c, which is passed NULL. But it doesn't really seem to use the callback data. It loops through the request queue to find requests that have yet to start (ie. their state is WAITING), and starts them if they haven't start yet. The request object (struct file_request) already contains a pointer to the callback data (walker), so it doesn't really need it.

Nevertheless, I'll make add_fill_function additionally check the callback data.

-- 
Cheers,
Ray Chuan