{"thread":{"id":"43226","subject":"Re: [ANNOUNCE] CGit v0.1-pre","startedAt":"2006-12-10T23:42:34Z","lastAt":"2006-12-11T18:38:57Z","messageCount":12,"participants":["Lars Hjemli","Jakub Narebski","Michael","Linus Torvalds"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"296661","messageId":"8c5c35580612101542n72d300c7o106e53e219670510@mail.gmail.com","threadId":"43226","inReplyTo":null,"subject":"[ANNOUNCE] CGit v0.1-pre","fromName":"Lars Hjemli","fromEmail":"hjemli@gmail.com","sentAt":"2006-12-10T23:42:34Z","receivedAt":"2006-12-10T23:42:34Z","isPatch":false,"sender":{"key":"hjemli@gmail.com","avatar":null},"body":"CGit is another cgi-app for git.\n\nIt is written in C (using libgit.a), and implements an internal page\ncache. A PoC version is available for cloning at\n\n git://hjemli.net/pub/git/cgit\n\nand it is also (barely) selfhosted at\n\n http://hjemli.net/git/\n\n\nFor a quick summary of the cache algorithm, the project README is here:\n\n http://hjemli.net/git/cgit/view/?id=5917c37ce30b3f0a374c9fa376955f51f1d7bfbf\n\nEnjoy!\n\n-- \n"},{"id":"295429","messageId":"eli7e3$srp$1@sea.gmane.org","threadId":"43226","inReplyTo":"8c5c35580612101542n72d300c7o106e53e219670510@mail.gmail.com","subject":"Re: [ANNOUNCE] CGit v0.1-pre","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-12-11T00:08:50Z","receivedAt":"2006-12-11T00:08:50Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Lars Hjemli wrote:\n\n> CGit is another cgi-app for git.\n\nI have added this to GitWiki: check out\n http://git.or.cz/gitwiki/InterfacesFrontendsAndTools#cgit\n\nHmmm... git has now 4 web interfaces (5 if counting gittracker, which works\nbut gittracker repository shows empty): gitweb in Perl, git-php in PHP, wit\nin Python (is it actively developed? where it can be downloaded from?),\ngitarella in Ruby, now CGit in C...\n\nBy the way, it is cgit, CGit or CGIt ;-p?\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n\n"},{"id":"294170","messageId":"200612110129.41721.jnareb@gmail.com","threadId":"43226","inReplyTo":"8c5c35580612101616g179715ecyd02fcbb023246ecc@mail.gmail.com","subject":"Re: [ANNOUNCE] CGit v0.1-pre","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-12-11T00:29:41Z","receivedAt":"2006-12-11T00:29:41Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Lars Hjemli wrote:\n> On 12/11/06, Jakub Narebski wrote:\n> \n>> Hmmm... git has now 4 web interfaces (5 if counting gittracker, which works\n>> but gittracker repository shows empty): gitweb in Perl, git-php in PHP, wit\n>> in Python (is it actively developed? where it can be downloaded from?),\n>> gitarella in Ruby, now CGit in C...\n> \n> Hmm, it must be a popular scm :-D\n\nMiscalculation: it is 5 with yours (6 counting gittracker).\n\n-- \nJakub Narebski\n"},{"id":"296707","messageId":"Pine.LNX.4.64.0612101653420.12500@woody.osdl.org","threadId":"43226","inReplyTo":"8c5c35580612101542n72d300c7o106e53e219670510@mail.gmail.com","subject":"Re: [ANNOUNCE] CGit v0.1-pre","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-12-11T01:04:15Z","receivedAt":"2006-12-11T01:04:15Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 11 Dec 2006, Lars Hjemli wrote:\n> \n> For a quick summary of the cache algorithm, the project README is here:\n> \n> http://hjemli.net/git/cgit/view/?id=5917c37ce30b3f0a374c9fa376955f51f1d7bfbf\n\nYour pseudo-algorithm is dubious:\n\n\t\tname = generate_cache_name(request);\n\ttop:\n\t\tif (!exists(name)) {\n\t\t\tif (lock_cache(name)) {\n\t\t\t\tgenerate_cache(request, name);\n\t\t\t\tunlock_cache(name);\n\t\t\t} else {\n\t\t\t\tsched_yield();\n\t\t\t\tgoto top;\n\t\t\t}\n\t\t} else if (expired(name)) {\n\t\t\tif (lock_cache(name)) {\n\t\t\t\tgenerate_cache(request, name);\n\t\t\t\tunlock_cache(name);\n\t\t\t}\n\t\t}\n\t\tprint_file(name);\n\n\nYou really should have:\n\n\tif (!exists) {\n\t\tif (!lock)\n\t\t\tdelay-and-repeat;\n\t\t/* RETEST exists _after_ getting the lock */\n\t\tif (!exists) {\n\t\t\tgenerate into lock-file\n\t\t\tmv lockfile exists;\n\t\t} else {\n\t\t\trm lockfile\n\t\t}\n\t}\n\nbecause you really want to re-check the existence after you got the lock, \notherwise you would race with somebody else that got the lock, generated \nthe data, and then unlocked (and you got the lock _after_ the data was \ngenerated, so now you generate it unnecessarily).\n\nAs a side note: how do you release your caches? \n\n"},{"id":"294077","messageId":"8c5c35580612110033y4f6260c9xffeaf4457f4ead21@mail.gmail.com","threadId":"43226","inReplyTo":"Pine.LNX.4.64.0612101653420.12500@woody.osdl.org","subject":"Re: [ANNOUNCE] CGit v0.1-pre","fromName":"Lars Hjemli","fromEmail":"hjemli@gmail.com","sentAt":"2006-12-11T08:33:36Z","receivedAt":"2006-12-11T08:33:36Z","isPatch":false,"sender":{"key":"hjemli@gmail.com","avatar":null},"body":"On 12/11/06, Linus Torvalds wrote:\n> you really want to re-check the existence after you got the lock,\n> otherwise you would race with somebody else that got the lock, generated\n> the data, and then unlocked (and you got the lock _after_ the data was\n> generated, so now you generate it unnecessarily).\n\nYes, you're right. Thanks for noticing.\n\nBut this also applies to the case where the cachefile has expired,\nright? In that case, after getting the lock, I have to recheck that\nthe cachefile is _still_ expired.\n\nAnyway, I must say I find it rather unlikely for these cases to occur\n(frequently) in real life. That would seem to imply that the caching\nisn't really needed at all.\n\n>\n> As a side note: how do you release your caches?\n>\n\nSimple timeouts (time()-stat.st_mtime), depending on what kind of page\nwas requested. If anyone cares about invalid cache content (branch\nhead moving), relevant cachefiles can be deleted with an update-hook.\n\n-- \n"},{"id":"297826","messageId":"8c5c35580612110359h6a8d0727ib669d773560e97f0@mail.gmail.com","threadId":"43226","inReplyTo":"8c5c35580612110033y4f6260c9xffeaf4457f4ead21@mail.gmail.com","subject":"Re: [ANNOUNCE] CGit v0.1-pre","fromName":"Lars Hjemli","fromEmail":"hjemli@gmail.com","sentAt":"2006-12-11T11:59:30Z","receivedAt":"2006-12-11T11:59:30Z","isPatch":false,"sender":{"key":"hjemli@gmail.com","avatar":null},"body":"On 12/11/06, Lars Hjemli wrote:\n> Anyway, I must say I find it rather unlikely for these cases to occur\n> (frequently) in real life.\n\n*blush*\n\nOf course this will happen in real life, I just needed to think about\nit for a while (it just depends on the _size_ of the thundering hurd,\nright?)\n\nAnyway, it's fixed (I think) and pushed out\n\n-- \n"},{"id":"296022","messageId":"200612111437.15054.barra_cuda@katamail.com","threadId":"43226","inReplyTo":"eli7e3$srp$1@sea.gmane.org","subject":"Re: [ANNOUNCE] CGit v0.1-pre","fromName":"Michael","fromEmail":"barra_cuda@katamail.com","sentAt":"2006-12-11T13:37:14Z","receivedAt":"2006-12-11T13:37:14Z","isPatch":false,"sender":{"key":"barra_cuda@katamail.com","avatar":"https://avatars.githubusercontent.com/u/16371673?v=4"},"body":"Jakub Narebski wrote:\n> wit\n> in Python (is it actively developed? where it can be downloaded from?),\n\nhere?\n\n"},{"id":"296676","messageId":"Pine.LNX.4.64.0612110859200.12500@woody.osdl.org","threadId":"43226","inReplyTo":"8c5c35580612110033y4f6260c9xffeaf4457f4ead21@mail.gmail.com","subject":"Re: [ANNOUNCE] CGit v0.1-pre","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-12-11T17:01:58Z","receivedAt":"2006-12-11T17:01:58Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 11 Dec 2006, Lars Hjemli wrote:\n>\n> But this also applies to the case where the cachefile has expired,\n> right? In that case, after getting the lock, I have to recheck that\n> the cachefile is _still_ expired.\n\nYes.\n\n> Anyway, I must say I find it rather unlikely for these cases to occur\n> (frequently) in real life. That would seem to imply that the caching\n> isn't really needed at all.\n\nThe point is, if you have races, you will hit them _occasionally_. It may \nnot be a performance problem in real life, BUT:\n\n - quite often, you can take advantage of the serialization guarantees \n that a front-side cache offers you, and do the uncached accesses (or \n writing the final cache-file) knowing that there's only one thing that \n does that.\n\n - If so, then a race condition in the cache goes from a \"unlikely \n performance problem\" to a BUG.\n\n> > As a side note: how do you release your caches?\n> \n> Simple timeouts (time()-stat.st_mtime), depending on what kind of page\n> was requested. If anyone cares about invalid cache content (branch\n> head moving), relevant cachefiles can be deleted with an update-hook.\n\nI was more thinking about the locking part. Again, to be safe, you should \nprobably take the lock before releasing any cache.\n\n"},{"id":"298078","messageId":"Pine.LNX.4.64.0612110929510.12500@woody.osdl.org","threadId":"43226","inReplyTo":"Pine.LNX.4.64.0612110859200.12500@woody.osdl.org","subject":"Re: [ANNOUNCE] CGit v0.1-pre","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-12-11T17:33:38Z","receivedAt":"2006-12-11T17:33:38Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 11 Dec 2006, Linus Torvalds wrote:\n> \n> The point is, if you have races, you will hit them _occasionally_. It may \n> not be a performance problem in real life, BUT:\n\nSide note: another problem with races is that\n\n - exactly because they are rare, any potential problems they cause are \n really hard to debug - you may have a hard time reproducing things.\n\n - some loads may be able to trigger them thanks to very unlucky timing, \n so even if the problem is \"just a theoretical performance issue\", \n sometimes that theoretical performance problem that only happens once \n in a blue moon ends up happening a _lot_ for a particular user.\n\nI'm probably biased, simply because I've done system programming for so \nlong (and race conditions etc are one of the most common source of subtle \nbugs), but I've long since come to the very strong opinion that locking is \nsimply too important to not do right.\n\nEven if you can argue that \"it doesn't matter\" (and you may even be \nright), I personally tend to just consider dodgy locking to be a serious \nbug _whether_ it really matters or not.\n\nJust a hangup of mine.\n\n"},{"id":"297710","messageId":"8c5c35580612110940p767fd360p52762a8818fbc8c1@mail.gmail.com","threadId":"43226","inReplyTo":"Pine.LNX.4.64.0612110859200.12500@woody.osdl.org","subject":"Re: [ANNOUNCE] CGit v0.1-pre","fromName":"Lars Hjemli","fromEmail":"hjemli@gmail.com","sentAt":"2006-12-11T17:40:48Z","receivedAt":"2006-12-11T17:40:48Z","isPatch":false,"sender":{"key":"hjemli@gmail.com","avatar":null},"body":"On 12/11/06, Linus Torvalds wrote:\n>\n> On Mon, 11 Dec 2006, Lars Hjemli wrote:\n> > Anyway, I must say I find it rather unlikely for these cases to occur\n> > (frequently) in real life. That would seem to imply that the caching\n> > isn't really needed at all.\n>\n> The point is, if you have races, you will hit them _occasionally_. It may\n> not be a performance problem in real life, BUT:\n>\n> - quite often, you can take advantage of the serialization guarantees\n> that a front-side cache offers you, and do the uncached accesses (or\n> writing the final cache-file) knowing that there's only one thing that\n> does that.\n>\n> - If so, then a race condition in the cache goes from a \"unlikely\n> performance problem\" to a BUG.\n\nYes, I finally understood (see my other reply)\n\n\n> > > As a side note: how do you release your caches?\n> >\n> > Simple timeouts (time()-stat.st_mtime), depending on what kind of page\n> > was requested. If anyone cares about invalid cache content (branch\n> > head moving), relevant cachefiles can be deleted with an update-hook.\n>\n> I was more thinking about the locking part. Again, to be safe, you should\n> probably take the lock before releasing any cache.\n\nOk. Code speeks louder than words, so I'll blatantly paste the key functions:\n\n--->8----\n\nconst int NOLOCK = -1;\n\nint cache_lock(struct cacheitem *item)\n{\n\tint i = 0;\n\tchar *lockfile = fmt(\"%s.lock\", item->name);\n\n top:\n\tif (++i > cgit_max_lock_attempts)\n\t\tdie(\"cache_lock: unable to lock %s: %s\",\n\t\t item->name, strerror(errno));\n\n \titem->fd = open(lockfile, O_WRONLY|O_CREAT|O_EXCL, S_IRUSR|S_IWUSR);\n\n\tif (item->fd == NOLOCK && errno == ENOENT && cache_create_dirs())\n\t\tgoto top;\n\n\tif (item->fd == NOLOCK && errno == EEXIST &&\n\t cache_refill_overdue(lockfile) && !unlink(lockfile))\n\t\t\tgoto top;\n\n\treturn (item->fd > 0);\n}\n\nint cache_unlock(struct cacheitem *item)\n{\n\tclose(item->fd);\n\treturn (rename(fmt(\"%s.lock\", item->name), item->name) == 0);\n}\n\n\nstatic void cgit_check_cache(struct cacheitem *item)\n{\n\tint i = 0;\n\n\tcache_prepare(item);\n top:\n\tif (++i > cgit_max_lock_attempts) {\n\t\tdie(\"cgit_refresh_cache: unable to lock %s: %s\",\n\t\t item->name, strerror(errno));\n\t}\n\tif (!cache_exist(item)) {\n\t\tif (!cache_lock(item)) {\n\t\t\tsleep(1);\n\t\t\tgoto top;\n\t\t}\n\t\tif (!cache_exist(item))\n\t\t\tcgit_fill_cache(item);\n\t\tcache_unlock(item);\n\t} else if (cache_expired(item) && cache_lock(item)) {\n\t\tif (cache_expired(item))\n\t\t\tcgit_fill_cache(item);\n\t\tcache_unlock(item);\n\t}\n}\n\n--->8----\n\nI am a bit conserned about the effect of cache_unlink() if another\nconcurrent process gets \"lucky\" with the test\n\"cache_refill_overdue(lockfile) && !unlink(lockfile)\".\n\nThis is supposed to be a safety valve against a stale lock file (lock\nfile not modified in n secs), but it doesn't feel quite right.\n\nProbably, if cache_unlink() fails in cgit_check_cache(), I should \"goto top\".\n\nOpinions?\n\n-- \n"},{"id":"298062","messageId":"Pine.LNX.4.64.0612111016220.12500@woody.osdl.org","threadId":"43226","inReplyTo":"8c5c35580612110940p767fd360p52762a8818fbc8c1@mail.gmail.com","subject":"Re: [ANNOUNCE] CGit v0.1-pre","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-12-11T18:18:32Z","receivedAt":"2006-12-11T18:18:32Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 11 Dec 2006, Lars Hjemli wrote:\n> \n> Ok. Code speeks louder than words, so I'll blatantly paste the key functions:\n\nYeah, they're still buggy:\n\n> int cache_unlock(struct cacheitem *item)\n> {\n> \tclose(item->fd);\n> \treturn (rename(fmt(\"%s.lock\", item->name), item->name) == 0);\n> }\n...\n> \tif (!cache_exist(item)) {\n> \t\tif (!cache_lock(item)) {\n> \t\t\tsleep(1);\n> \t\t\tgoto top;\n> \t\t}\n> \t\tif (!cache_exist(item))\n> \t\t\tcgit_fill_cache(item);\n> \t\tcache_unlock(item);\n\nWhat do you think happens if that last \"cache_exist()\" returned true?\n\nThat's right: the \"cache_unlock()\" will now OVERWRITE the valid cache with \nthe (empty) lock-file that you didn't fill in.\n\nOops.\n\nSo you really have two different cases:\n\n - the \"I created the file\" case: rename lockfile to final name\n - the \"somebody else created the file\": remove the lockfile\n\nand you can't use the same \"cache_unlock()\" for both of them.\n\n"},{"id":"298402","messageId":"8c5c35580612111038u480b1f95xdb7cf1b3b8fd97b@mail.gmail.com","threadId":"43226","inReplyTo":"Pine.LNX.4.64.0612111016220.12500@woody.osdl.org","subject":"Re: [ANNOUNCE] CGit v0.1-pre","fromName":"Lars Hjemli","fromEmail":"hjemli@gmail.com","sentAt":"2006-12-11T18:38:57Z","receivedAt":"2006-12-11T18:38:57Z","isPatch":false,"sender":{"key":"hjemli@gmail.com","avatar":null},"body":"On 12/11/06, Linus Torvalds wrote:\n>\n>\n> On Mon, 11 Dec 2006, Lars Hjemli wrote:\n> >\n> > Ok. Code speeks louder than words, so I'll blatantly paste the key functions:\n>\n> Yeah, they're still buggy:\n>\n> > int cache_unlock(struct cacheitem *item)\n> > {\n> > close(item->fd);\n> > return (rename(fmt(\"%s.lock\", item->name), item->name) == 0);\n> > }\n> ...\n> > if (!cache_exist(item)) {\n> > if (!cache_lock(item)) {\n> > sleep(1);\n> > goto top;\n> > }\n> > if (!cache_exist(item))\n> > cgit_fill_cache(item);\n> > cache_unlock(item);\n>\n> What do you think happens if that last \"cache_exist()\" returned true?\n>\n> That's right: the \"cache_unlock()\" will now OVERWRITE the valid cache with\n> the (empty) lock-file that you didn't fill in.\n>\n> Oops.\n>\n\nWow, that's embarrassing.\n\nThanks for the debugging, I'll try to actually fix it this time :-)\n\n-- \n"}]}