{"thread":{"id":"18189","subject":"[PATCH] http: add_fill_function checks if function has been added","startedAt":"2009-03-07T12:21:04Z","lastAt":"2009-03-09T12:01:32Z","messageCount":7,"participants":["Tay Ray Chuan","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"107289","messageId":"49B266B0.4020304@gmail.com","threadId":"18189","inReplyTo":null,"subject":"[PATCH] http: add_fill_function checks if function has been added","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-03-07T12:21:04Z","receivedAt":"2009-03-07T12:21:04Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"This patch ensures that the same fill function is called once so to\nprevent any possible issues.\n\nNevertheless, calling a fill function repeatedly in\n''fill_active_slots'' will not lead to any obvious change in existing\nbehavior, though performance might be affected.\n\n''add_fill_action'' checks if the function to be added has already been\nadded. Allocation of memory for the list ''fill_chain*'' is postponed\nuntil the check passes, unlike previously.\n\nSigned-off-by: Tay Ray Chuan \n---\n http.c | 10 +++++++---\n 1 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex ee58799..cdedeb6 100644\n--- a/http.c\n+++ b/http.c\n@@ -408,13 +408,17 @@ static struct fill_chain *fill_cfg = NULL;\n\n void add_fill_function(void *data, int (*fill)(void *))\n {\n-\tstruct fill_chain *new = xmalloc(sizeof(*new));\n+\tstruct fill_chain *new;\n \tstruct fill_chain **linkp = &fill_cfg;\n+\tfor (;*linkp; linkp = &(*linkp)->next)\n+\t\tif ((*linkp)->fill == fill)\n+\t\t\treturn;\n+\n+\tnew = xmalloc(sizeof(*new));\n \tnew->data = data;\n \tnew->fill = fill;\n \tnew->next = NULL;\n-\twhile (*linkp)\n-\t\tlinkp = &(*linkp)->next;\n+\n \t*linkp = new;\n }\n\n-- \n1.6.2.rc1\n"},{"id":"107337","messageId":"7vy6vhm6it.fsf@gitster.siamese.dyndns.org","threadId":"18189","inReplyTo":"49B266B0.4020304@gmail.com","subject":"Re: [PATCH] http: add_fill_function checks if function has been added","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-07T20:18:02Z","receivedAt":"2009-03-07T20:18:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tay Ray Chuan writes:\n\n> This patch ensures that the same fill function is called once so to\n> prevent any possible issues.\n>\n> Nevertheless, calling a fill function repeatedly in\n> ''fill_active_slots'' will not lead to any obvious change in existing\n> behavior, though performance might be affected.\n>\n> ''add_fill_action'' checks if the function to be added has already been\n> added. Allocation of memory for the list ''fill_chain*'' is postponed\n> until the check passes, unlike previously.\n\nCould you care to explain the following a bit better?\n\n - what \"possible issues\" you are addressing;\n\n - what changes in the behaviour that are not \"obvious\" we would be\n suffering from, if we apply this patch;\n\n - in what situation the performance _might_ be affected, in what way and\n to what extent.\n\nIf the patch author does not have clear answers to these questions, how\ncan others decide if it is worth reading the patch to judge if it is worth\napplying?\n\nIn other words, I'd expect you to explain the issues like this:\n\n add_fill_function() adds the same fill function twice on the fill_cfg\n list; this causes THIS and THAT breakages when the fill function is\n called twice.\n\n Ignore add_fill_function() when fill_cfg list already has the function\n registered on it to fix this issue. Note however that the new code may\n behave very inefficiently under this situation:\n\n - XYZZY happens, then\n - FROTZ happens, and then\n - NITFOL happens.\n\n In such a case we end up doing FROTZ repeatedly, and ...; we might\n want to later optimize this, but a correctly working code is more\n important than efficient code that works most of the time but silently\n breaks in some cases.\n\n We need to iterate over the existing entries in fill_cfg list to find\n duplicates, which may look like an overhead, but the existing function\n already needed to do so to queue the new entry at the end anyway, so\n this is nothing new.\n"},{"id":"107341","messageId":"be6fef0d0903071249s42ac7f94o82461ca32dcdfcd5@mail.gmail.com","threadId":"18189","inReplyTo":"7vy6vhm6it.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] http: add_fill_function checks if function has been added","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-03-07T20:49:13Z","receivedAt":"2009-03-07T20:49:13Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Hi,\n\nOn Sun, Mar 8, 2009 at 4:18 AM, Junio C Hamano wrote:\n> Could you care to explain the following a bit better?\n>\n>  - what \"possible issues\" you are addressing;\n>\n>  - what changes in the behaviour that are not \"obvious\" we would be\n>   suffering from, if we apply this patch;\n>\n>  - in what situation the performance _might_ be affected, in what way and\n>   to what extent.\n\n(note: \"repeatedly\" here means looping over it, eg. while(condition)\nfill_function(). )\n\nThanks for taking the time to give this clear and detailed example explanation.\n\nHowever, at this point of time, I couldn't come up with a convincing\ninstance of where\n\n *a fill function is added twice or more, and as a result\n\n *something breaks as a result of invoking the function repeatedly\n\nthat was why I used the word \"possible\" as in \"possible issues\",\nbecause this patch doesn't solve any existing issues (at least none\nthat I know of now).\n\nCalling a fill function repeatedly won't break behaviour, because fill\nfunctions (those that are currently defined in git) are designed to be\ncalled repeatedly. But it's just useless to call the same fill\nfunction repeatedly without any reason.\n\nSo should I still address the \"THIS and THAT breakages\"?\n\n-- \nCheers,\nRay Chuan\n"},{"id":"107351","messageId":"7vd4ctm29m.fsf@gitster.siamese.dyndns.org","threadId":"18189","inReplyTo":"be6fef0d0903071249s42ac7f94o82461ca32dcdfcd5@mail.gmail.com","subject":"Re: [PATCH] http: add_fill_function checks if function has been added","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-07T21:49:57Z","receivedAt":"2009-03-07T21:49:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tay Ray Chuan writes:\n\n> Calling a fill function repeatedly won't break behaviour, because fill\n> functions (those that are currently defined in git) are designed to be\n> called repeatedly. But it's just useless to call the same fill\n> function repeatedly without any reason.\n>\n> So should I still address the \"THIS and THAT breakages\"?\n\nYour above explanation is good, but it was given after I asked ;-).\nMaking unnecessary call repeatedly counts as a breakage your patch fixes,\nright?\n\nI didn't look at the callers of add_fill_function(), but \"fill\" takes a\ncallback data and different invocation of add_fill_function() could be\npassing different callback data. In such a case, doesn't it feel wrong to\nomit the \"duplicated\" calls to register the fill callback? Your patch\nmakes me suspect that it _might_ be better to fix the callers not to call\nthe function repeatedly when they know they only want one-shot invocation.\n"},{"id":"107376","messageId":"be6fef0d0903080327u551c0b4mcb86f2ba76473efc@mail.gmail.com","threadId":"18189","inReplyTo":"7vd4ctm29m.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] http: add_fill_function checks if function has been added","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-03-08T10:27:17Z","receivedAt":"2009-03-08T10:27:17Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Hi,\n\nOn Sun, Mar 8, 2009 at 5:49 AM, Junio C Hamano wrote:\n> I didn't look at the callers of add_fill_function(), but \"fill\" takes a\n> callback data and different invocation of add_fill_function() could be\n> passing different callback data.  In such a case, doesn't it feel wrong to\n> omit the \"duplicated\" calls to register the fill callback?  Your patch\n> makes me suspect that it _might_ be better to fix the callers not to call\n> the function repeatedly when they know they only want one-shot invocation.\n\nOmitting duplicate calls in fill_active_slots does not mean that\nrepeated calls of the fill functions won't take place.\n\nIn the current use instances in http-push and http-walker,\nrun_active_slot and finish_all_active_slots (which calls\nrun_active_slot) will always be called, because they are the functions\nthat actually start the http requests[1], and that means the fill\nfunctions will be called repeated, because\n\nrun_active_slot, in its while loop, repeatedly calls\n step_active_slots calls\n fill_active_slots calls\n our fill functions.\n\nfill_active_slots is the only direct caller of fill functions\n(duplicate ones included).\n\nSo we can be sure that the fill functions are called repeatedly (at\nleast for the slot's active duration).\n\n[1] Actually calling step_active_slots will start http requests,\nwithout you having to call run_active_slot.\n\n-- \nCheers,\nRay Chuan\n"},{"id":"107394","messageId":"7vd4crls8q.fsf@gitster.siamese.dyndns.org","threadId":"18189","inReplyTo":"be6fef0d0903080327u551c0b4mcb86f2ba76473efc@mail.gmail.com","subject":"Re: [PATCH] http: add_fill_function checks if function has been added","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-08T19:38:45Z","receivedAt":"2009-03-08T19:38:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tay Ray Chuan writes:\n\n> In the current use instances in http-push and http-walker,\n> run_active_slot and finish_all_active_slots (which calls\n> run_active_slot) will always be called, because they are the functions\n> that actually start the http requests[1], and that means the fill\n> functions will be called repeated, because\n>\n> run_active_slot, in its while loop, repeatedly calls\n> step_active_slots calls\n> fill_active_slots calls\n> our fill functions.\n\nThat is not what I was worried about. There are two callsites to\nadd_fill_function().\n\nhttp-push.c:2439:\t\tadd_fill_function(NULL, fill_active_slot);\nhttp-walker.c:936:\tadd_fill_function(walker, (int (*)(void *)) fill_active_slot);\n\nThe caller in http-push.c always passes NULL as the callback data for the\nfill_active_slot callback, which means that the callback function, as long\nas it does not depend on the state other than the callback function\nparameter (which is always NULL), will do the same thing whether you queue\nit only once or as many times as add_fill_function() was called, which the\nimprovement your patch brings in.\n\nBut it is not immediately obvious in http-walker.c callchain if multiple\ncalls to add_fill_function(), if they were ever made, give the same\n\"walker\" as the callback data. Your patch only looks at the function but\nnot the callback data for its filtering. Doesn't it mean that if a caller\nmade two calls to add_fill_function() with walker#1 and then walker#2 as\nthe callback data, you would discard the request for walker#2 and call the\ncallback function fill_active_slot() with walker#1 inside run_active_slot\nrepeatedly, without ever calling it for walker#2?\n\nI do not know if that actually happens in the current http-walker.c\ncodepath, or the caller _happens to_ call the add_fill_function() only\nonce and making my worry a non-issue, but as a patch submitter, you would\nknow the answer to the question.\n\nIf it is the former, then your change is introducing a bug. If it is the\nlatter, your change won't break anything immediately, but still introduces\na potential bug waiting to happen, as the API looks as if you can call\nadd_fill_function() to ask for callback functions to be called with\ndifferent callback data and the caller can rely on both of them to be\ncalled at appropriate times, but in reality that guarantee does not hold.\n"},{"id":"107452","messageId":"be6fef0d0903090501m21ee33b6pc3b45680324d932c@mail.gmail.com","threadId":"18189","inReplyTo":"7vd4crls8q.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] http: add_fill_function checks if function has been added","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-03-09T12:01:32Z","receivedAt":"2009-03-09T12:01:32Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Hi,\n\nOn Mon, Mar 9, 2009 at 3:38 AM, Junio C Hamano wrote:\n> That is not what I was worried about. There are two callsites to\n> add_fill_function().\n\nI'm sorry I didn't catch this from your earlier message; on a\nre-reading I caught it.\n\n> But it is not immediately obvious in http-walker.c callchain if multiple\n> calls to add_fill_function(), if they were ever made, give the same\n> \"walker\" as the callback data. Your patch only looks at the function but\n> not the callback data for its filtering. Doesn't it mean that if a caller\n> made two calls to add_fill_function() with walker#1 and then walker#2 as\n> the callback data, you would discard the request for walker#2 and call the\n> callback function fill_active_slot() with walker#1 inside run_active_slot\n> repeatedly, without ever calling it for walker#2?\n\nYou're right, in addition to checking whether the callback is the\nsame, I should also have checked the callback data, before discarding\nthe invocation of add_fill_function as a duplicate.\n\n> If it is the former, then your change is introducing a bug. If it is the\n> latter, your change won't break anything immediately, but still introduces\n> a potential bug waiting to happen, as the API looks as if you can call\n> add_fill_function() to ask for callback functions to be called with\n> different callback data and the caller can rely on both of them to be\n> called at appropriate times, but in reality that guarantee does not hold.\n\nThat indeed is true, but afaik, none of the two instances of the API\nusage does that -- calling callback functions with different callback\ndata to change behaviour. The fill functions are basically used to\n\"clear\" the request queue.\n\nLook at the fill function in http-walker.c. Yes, it's true that\ncallback data is passed on by the fill function, unlike the one in\nhttp-push.c, which is passed NULL. But it doesn't really seem to use\nthe callback data. It loops through the request queue to find requests\nthat have yet to start (ie. their state is WAITING), and starts them\nif they haven't start yet. The request object (struct file_request)\nalready contains a pointer to the callback data (walker), so it\ndoesn't really need it.\n\nNevertheless, I'll make add_fill_function additionally check the callback data.\n\n-- \nCheers,\nRay Chuan\n"}]}