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

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

[Tay Ray Chuan]

This patch ensures that the same fill function is called once so to
prevent any possible issues.

Nevertheless, calling a fill function repeatedly in
''fill_active_slots'' will not lead to any obvious change in existing
behavior, though performance might be affected.

''add_fill_action'' checks if the function to be added has already been
added. Allocation of memory for the list ''fill_chain*'' is postponed
until the check passes, unlike previously.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 http.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/http.c b/http.c
index ee58799..cdedeb6 100644
--- a/http.c
+++ b/http.c
@@ -408,13 +408,17 @@ static struct fill_chain *fill_cfg = NULL;

 void add_fill_function(void *data, int (*fill)(void *))
 {
-	struct fill_chain *new = xmalloc(sizeof(*new));
+	struct fill_chain *new;
 	struct fill_chain **linkp = &fill_cfg;
+	for (;*linkp; linkp = &(*linkp)->next)
+		if ((*linkp)->fill == fill)
+			return;
+
+	new = xmalloc(sizeof(*new));
 	new->data = data;
 	new->fill = fill;
 	new->next = NULL;
-	while (*linkp)
-		linkp = &(*linkp)->next;
+
 	*linkp = new;
 }

-- 
1.6.2.rc1

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

[Junio C Hamano]

Tay Ray Chuan <rctay89@gmail.com> writes:

> This patch ensures that the same fill function is called once so to
> prevent any possible issues.
>
> Nevertheless, calling a fill function repeatedly in
> ''fill_active_slots'' will not lead to any obvious change in existing
> behavior, though performance might be affected.
>
> ''add_fill_action'' checks if the function to be added has already been
> added. Allocation of memory for the list ''fill_chain*'' is postponed
> until the check passes, unlike previously.

Could you care to explain the following a bit better?

 - what "possible issues" you are addressing;

 - what changes in the behaviour that are not "obvious" we would be
   suffering from, if we apply this patch;

 - in what situation the performance _might_ be affected, in what way and
   to what extent.

If the patch author does not have clear answers to these questions, how
can others decide if it is worth reading the patch to judge if it is worth
applying?

In other words, I'd expect you to explain the issues like this:

    add_fill_function() adds the same fill function twice on the fill_cfg
    list; this causes THIS and THAT breakages when the fill function is
    called twice.

    Ignore add_fill_function() when fill_cfg list already has the function
    registered on it to fix this issue.  Note however that the new code may
    behave very inefficiently under this situation:

    - XYZZY happens, then
    - FROTZ happens, and then
    - NITFOL happens.

    In such a case we end up doing FROTZ repeatedly, and ...; we might
    want to later optimize this, but a correctly working code is more
    important than efficient code that works most of the time but silently
    breaks in some cases.

    We need to iterate over the existing entries in fill_cfg list to find
    duplicates, which may look like an overhead, but the existing function
    already needed to do so to queue the new entry at the end anyway, so
    this is nothing new.

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

[Tay Ray Chuan]

Hi,

On Sun, Mar 8, 2009 at 4:18 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Could you care to explain the following a bit better?
>
>  - what "possible issues" you are addressing;
>
>  - what changes in the behaviour that are not "obvious" we would be
>   suffering from, if we apply this patch;
>
>  - in what situation the performance _might_ be affected, in what way and
>   to what extent.

(note: "repeatedly" here means looping over it, eg. while(condition)
fill_function(). )

Thanks for taking the time to give this clear and detailed example explanation.

However, at this point of time, I couldn't come up with a convincing
instance of where

 *a fill function is added twice or more, and as a result

 *something breaks as a result of invoking the function repeatedly

that was why I used the word "possible" as in "possible issues",
because this patch doesn't solve any existing issues (at least none
that I know of now).

Calling a fill function repeatedly won't break behaviour, because fill
functions (those that are currently defined in git) are designed to be
called repeatedly. But it's just useless to call the same fill
function repeatedly without any reason.

So should I still address the "THIS and THAT breakages"?

-- 
Cheers,
Ray Chuan

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

[Junio C Hamano]

Tay Ray Chuan <rctay89@gmail.com> writes:

> Calling a fill function repeatedly won't break behaviour, because fill
> functions (those that are currently defined in git) are designed to be
> called repeatedly. But it's just useless to call the same fill
> function repeatedly without any reason.
>
> So should I still address the "THIS and THAT breakages"?

Your above explanation is good, but it was given after I asked ;-).
Making unnecessary call repeatedly counts as a breakage your patch fixes,
right?

I didn't look at the callers of add_fill_function(), but "fill" takes a
callback data and different invocation of add_fill_function() could be
passing different callback data.  In such a case, doesn't it feel wrong to
omit the "duplicated" calls to register the fill callback?  Your patch
makes me suspect that it _might_ be better to fix the callers not to call
the function repeatedly when they know they only want one-shot invocation.

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

[Tay Ray Chuan]

Hi,

On Sun, Mar 8, 2009 at 5:49 AM, Junio C Hamano <gitster@pobox.com> wrote:
> I didn't look at the callers of add_fill_function(), but "fill" takes a
> callback data and different invocation of add_fill_function() could be
> passing different callback data.  In such a case, doesn't it feel wrong to
> omit the "duplicated" calls to register the fill callback?  Your patch
> makes me suspect that it _might_ be better to fix the callers not to call
> the function repeatedly when they know they only want one-shot invocation.

Omitting duplicate calls in fill_active_slots does not mean that
repeated calls of the fill functions won't take place.

In the current use instances in http-push and http-walker,
run_active_slot and finish_all_active_slots (which calls
run_active_slot) will always be called, because they are the functions
that actually start the http requests[1], and that means the fill
functions will be called repeated, because

run_active_slot, in its while loop, repeatedly calls
  step_active_slots calls
    fill_active_slots calls
      our fill functions.

fill_active_slots is the only direct caller of fill functions
(duplicate ones included).

So we can be sure that the fill functions are called repeatedly (at
least for the slot's active duration).

[1] Actually calling step_active_slots will start http requests,
without you having to call run_active_slot.

-- 
Cheers,
Ray Chuan

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

[Junio C Hamano]

Tay Ray Chuan <rctay89@gmail.com> writes:

> In the current use instances in http-push and http-walker,
> run_active_slot and finish_all_active_slots (which calls
> run_active_slot) will always be called, because they are the functions
> that actually start the http requests[1], and that means the fill
> functions will be called repeated, because
>
> run_active_slot, in its while loop, repeatedly calls
>   step_active_slots calls
>     fill_active_slots calls
>       our fill functions.

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

http-push.c:2439:		add_fill_function(NULL, fill_active_slot);
http-walker.c:936:	add_fill_function(walker, (int (*)(void *)) fill_active_slot);

The caller in http-push.c always passes NULL as the callback data for the
fill_active_slot callback, which means that the callback function, as long
as it does not depend on the state other than the callback function
parameter (which is always NULL), will do the same thing whether you queue
it only once or as many times as add_fill_function() was called, which the
improvement your patch brings in.

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?

I do not know if that actually happens in the current http-walker.c
codepath, or the caller _happens to_ call the add_fill_function() only
once and making my worry a non-issue, but as a patch submitter, you would
know the answer to the question.

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.

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

[Tay Ray Chuan]

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.

> 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.

> 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