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.
* 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
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:
-
-<pre>
-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 (<IN>) {
-</pre>
+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]]
sipb-www / ikiwiki.git / blobdiff