X-Git-Url: http://git.vanrenterghem.biz/git.ikiwiki.info.git/blobdiff_plain/9586107d138fc2b88e9960e0f7f11d8dce1dc468..fdb4fad223cac4b5a87f7da2de9fc7dc1b9a8f8b:/doc/bugs/locking_fun.mdwn?ds=inline diff --git a/doc/bugs/locking_fun.mdwn b/doc/bugs/locking_fun.mdwn index 5ecf9f846..46278b028 100644 --- a/doc/bugs/locking_fun.mdwn +++ b/doc/bugs/locking_fun.mdwn @@ -7,6 +7,8 @@ This can happen because CGI.pm writes the change, then drops the main wiki lock before calling rcs_commit. It can't keep the lock because the commit hook needs to be able to lock. +------- + We batted this around for an hour or two on irc. The best solution seems to be adding a subsidiary second lock, which is only used to lock the working copy and is a blocking read/write lock. @@ -14,8 +16,8 @@ copy and is a blocking read/write lock. * As before, the CGI will take the main wiki lock when starting up. * Before writing to the WC, the CGI takes an exclusive lock on the WC. * After writing to the WC, the CGI can downgrade it to a shared lock. - (This downgrade has to happen atomically, to prevent other CGIs from - stealing the exclusive lock.) + (If this downgrade does not happen atomically, other CGIs can + steal the exclusive lock.) * Then the CGI, as before, drops the main wiki lock to prevent deadlock. It keeps its shared WC lock. * The commit hook takes first the main wiki lock and then the shared WC lock @@ -24,97 +26,80 @@ copy and is a blocking read/write lock. the main wiki lock (that could deadlock). It does its final stuff and exits, dropping the shared WC lock. -Sample patch, with stub functions for the new lock: - -
-Index: IkiWiki/CGI.pm -=================================================================== ---- IkiWiki/CGI.pm (revision 2774) -+++ IkiWiki/CGI.pm (working copy) -@@ -494,9 +494,14 @@ - $content=~s/\r\n/\n/g; - $content=~s/\r/\n/g; - -+ lockwc_exclusive(); -+ - $config{cgi}=0; # avoid cgi error message - eval { writefile($file, $config{srcdir}, $content) }; - $config{cgi}=1; -+ -+ lockwc_shared(); -+ - if ($@) { - $form->field(name => "rcsinfo", value => rcs_prepedit($file), - force => 1); -Index: IkiWiki/Plugin/poll.pm -=================================================================== ---- IkiWiki/Plugin/poll.pm (revision 2770) -+++ IkiWiki/Plugin/poll.pm (working copy) -@@ -120,7 +120,9 @@ - $content =~ s{(\\?)\[\[poll\s+([^]]+)\s*\]\]}{$edit->($1, $2)}seg; - - # Store their vote, update the page, and redirect to it. -+ IkiWiki::lockwc_exclusive(); - writefile($pagesources{$page}, $config{srcdir}, $content); -+ IkiWiki::lockwc_shared(); - $session->param($choice_param, $choice); - IkiWiki::cgi_savesession($session); - $oldchoice=$session->param($choice_param); -@@ -130,6 +132,10 @@ - IkiWiki::rcs_commit($pagesources{$page}, "poll vote ($choice)", - IkiWiki::rcs_prepedit($pagesources{$page}), - $session->param("name"), $ENV{REMOTE_ADDR}); -+ # Make sure that the repo is up-to-date; -+ # locking prevents the post-commit hook -+ # from updating it. -+ rcs_update(); - } - else { - require IkiWiki::Render; -Index: ikiwiki.in -=================================================================== ---- ikiwiki.in (revision 2770) -+++ ikiwiki.in (working copy) -@@ -121,6 +121,7 @@ - lockwiki(); - loadindex(); - require IkiWiki::Render; -+ lockwc_shared(); - rcs_update(); - refresh(); - rcs_notify() if $config{notify}; -Index: IkiWiki.pm -=================================================================== ---- IkiWiki.pm (revision 2770) -+++ IkiWiki.pm (working copy) -@@ -617,6 +617,29 @@ - close WIKILOCK; - } #}}} - -+sub lockwc_exclusive () { #{{{ -+ # Take an exclusive lock on the working copy. -+ # The lock will be dropped on program exit. -+ # Note: This lock should only be taken _after_ the main wiki -+ # lock. -+ -+ # TODO -+} #}}} -+ -+sub lockwc_shared () { #{{{ -+ # Take a shared lock on the working copy. If an exclusive lock -+ # already exists, downgrade it to a shared lock. -+ # The lock will be dropped on program exit. -+ # Note: This lock should only be taken _after_ the main wiki -+ # lock. -+ -+ # TODO -+} #}}} -+ -+sub unlockwc () { #{{{ -+ close WIKIWCLOCK; -+} #}}} -+ - sub loadindex () { #{{{ - open (IN, "$config{wikistatedir}/index") || return; - while (+Locking: + +Using fcntl locking from perl is very hard. flock locking has the problem +that one some OSes (linux?) converting an exclusive to a shared lock is not +atomic and can be raced. What happens if this race occurs is that, +since ikiwiki always uses LOCK_NB, the flock fails. Then we're back to the +original race. It should be possible though to use a separate exclusive lock, +wrapped around these flock calls, to force them to be "atomic" and avoid that +race. + +------ + +My alternative idea, which seems simpler than all this tricky locking +stuff, is to introduce a new lock file (really a flag file implemented +using a lock), which tells the commit hook that the CGI is running, and +makes the commit hook a NOOP. + +* CGI takes the wikilock +* CGI writes changes to WC +* CGI sets wclock to disable the commit hook +* CGI does *not* drop the main wikilock +* CGI commit +* The commit hook tries to set the wclock, fails, and becomes a noop + (it may still need to send commit mails) +* CGI removes wclock, thus re-enabling the commit hook +* CGI updates the WC (since the commit hook didn't) +* CGI renders the wiki (always. commits may have came in and not been + rendered) +* CGI checks for conflicts, and if any are found does its normal dance + +> It seems like there are two things to be concerned with: RCS commit between +> disable of hook and CGI commit, or RCS commit between CGI commit and re-enable +> of hook. The second case isn't a big deal if the CGI is gonna rerender +> everything anyhow. --[[Ethan]] + +I agree, and I think that the second case points to the hooks still being +responsible for sending out commit mails. Everything else the CGI can do. + +I don't believe that the first case is actually a problem: If the RCS +commit does not introduce a conflict then the CGI commit's changes will be +merged into the repo cleanly. OTOH, if the RCS commit does introduces a +conflict then the CGI commit will fail gracefully. This is exactly what +happens now if RCS commit happens while a CGI commit is in progress! Ie: + +* cgi takes the wikilock +* cgi writes change to wc +* svn commit -m "conflict" (this makes a change to repo immediately, then + runs the post-commit hook, which waits on the wikilock) +* cgi drops wikilock +* the post-commit hook from the above manual commit can now run. +* cgi calls rcs_commit, which fails due to the conflict just introduced + +The only difference to this scenario will be that the CGI will not drop the +wiki lock before its commit, and that the post-commit hook will turn into a +NOOP: + +* cgi takes the wikilock +* cgi writes change to wc +* cgi takes the wclock +* svn commit -m "conflict" (this makes a change to repo immediately, then + runs the post-commit hook, which becomes a NOOP) +* cgi calls rcs_commit, which fails due to the conflict just introduced +* cgi renders the wiki + +Actually, the only thing that scares me about this apprach a little is that +we have two locks. The CGI takes them in the order (wikilock, wclock). +The commit hook takes them in the order (wclock, wikilock). This is a +classic potential deadlock scenario. _However_, the commit hook should +close the wclock as soon as it successfully opens it, before taking the +wikilock, so I think that's ok. + +----- + +I've committed an implementation of my idea just above, and it seems to +work, although testing for races etc is tricky. Calling this [[bugs/done]] +unless someone finds a new bug or finds a problem in my thinking above. +--[[Joey]]) { -