git.oblomov.eu Git - ikiwiki/blob - doc/bugs/locking_fun.mdwn

   1 It's possible for concurrent web edits to race and the winner commits both
   2 changes at once with its commit message (see r2779). The loser gets a
   3 message that there were conflicts and gets to see his own edits as the
   4 conflicting edits he's supposed to resolve.
   5
   6 This can happen because CGI.pm writes the change, then drops the main wiki
   7 lock before calling rcs_commit. It can't keep the lock because the commit
   8 hook needs to be able to lock.
   9
  10 We batted this around for an hour or two on irc. The best solution seems to
  11 be adding a subsidiary second lock, which is only used to lock the working
  12 copy and is a blocking read/write lock.
  13
  14 * As before, the CGI will take the main wiki lock when starting up.
  15 * Before writing to the WC, the CGI takes an exclusive lock on the WC.
  16 * After writing to the WC, the CGI can downgrade it to a shared lock.
  17   (If this downgrade does not happen atomically, other CGIs can
  18   steal the exclusive lock.)
  19 * Then the CGI, as before, drops the main wiki lock to prevent deadlock. It
  20   keeps its shared WC lock.
  21 * The commit hook takes first the main wiki lock and then the shared WC lock
  22   when starting up, and holds them until it's done.
  23 * Once the commit is done, the CGI, as before, does not attempt to regain
  24   the main wiki lock (that could deadlock). It does its final stuff and
  25   exits, dropping the shared WC lock.
  26
  27 Locking:
  28
  29 Using fcntl locking from perl is very hard. flock locking has the problem
  30 that one some OSes (linux?) converting an exclusive to a shared lock is not
  31 atomic and can be raced. What happens if this race occurs is that,
  32 since ikiwiki always uses LOCK_NB, the flock fails. Then we're back to the
  33 original race. It should be possible though to use a separate exclusive lock,
  34 wrapped around these flock calls, to force them to be "atomic" and avoid that
  35 race.
  36
  37 Sample patch, with stub functions for the new lock:
  38
  39 [[toggle text="expand patch"]]
  40 [[toggleable text="""
  41 <pre>
  42 Index: IkiWiki/CGI.pm
  43 ===================================================================
  44 --- IkiWiki/CGI.pm      (revision 2774)
  45 +++ IkiWiki/CGI.pm      (working copy)
  46 @@ -494,9 +494,14 @@
  47                 $content=~s/\r\n/\n/g;
  48                 $content=~s/\r/\n/g;
  49
  50 +               lockwc_exclusive();
  51 +
  52                 $config{cgi}=0; # avoid cgi error message
  53                 eval { writefile($file, $config{srcdir}, $content) };
  54                 $config{cgi}=1;
  55 +
  56 +               lockwc_shared();
  57 +
  58                 if ($@) {
  59                         $form->field(name => "rcsinfo", value => rcs_prepedit($file),
  60                                 force => 1);
  61 Index: IkiWiki/Plugin/poll.pm
  62 ===================================================================
  63 --- IkiWiki/Plugin/poll.pm      (revision 2770)
  64 +++ IkiWiki/Plugin/poll.pm      (working copy)
  65 @@ -120,7 +120,9 @@
  66                 $content =~ s{(\\?)\[\[poll\s+([^]]+)\s*\]\]}{$edit->($1, $2)}seg;
  67
  68                 # Store their vote, update the page, and redirect to it.
  69 +               IkiWiki::lockwc_exclusive();
  70                 writefile($pagesources{$page}, $config{srcdir}, $content);
  71 +               IkiWiki::lockwc_shared();
  72                 $session->param($choice_param, $choice);
  73                 IkiWiki::cgi_savesession($session);
  74                 $oldchoice=$session->param($choice_param);
  75 @@ -130,6 +132,10 @@
  76                         IkiWiki::rcs_commit($pagesources{$page}, "poll vote ($choice)",
  77                                 IkiWiki::rcs_prepedit($pagesources{$page}),
  78                                 $session->param("name"), $ENV{REMOTE_ADDR});
  79 +                       # Make sure that the repo is up-to-date;
  80 +                       # locking prevents the post-commit hook
  81 +                       # from updating it.
  82 +                       rcs_update();
  83                 }
  84                 else {
  85                         require IkiWiki::Render;
  86 Index: ikiwiki.in
  87 ===================================================================
  88 --- ikiwiki.in  (revision 2770)
  89 +++ ikiwiki.in  (working copy)
  90 @@ -121,6 +121,7 @@
  91                 lockwiki();
  92                 loadindex();
  93                 require IkiWiki::Render;
  94 +               lockwc_shared();
  95                 rcs_update();
  96                 refresh();
  97                 rcs_notify() if $config{notify};
  98 Index: IkiWiki.pm
  99 ===================================================================
 100 --- IkiWiki.pm  (revision 2770)
 101 +++ IkiWiki.pm  (working copy)
 102 @@ -617,6 +617,29 @@
 103         close WIKILOCK;
 104  } #}}}
 105
 106 +sub lockwc_exclusive () { #{{{
 107 +       # Take an exclusive lock on the working copy.
 108 +       # The lock will be dropped on program exit.
 109 +       # Note: This lock should only be taken _after_ the main wiki
 110 +       # lock.
 111 +
 112 +       # TODO
 113 +} #}}}
 114 +
 115 +sub lockwc_shared () { #{{{
 116 +       # Take a shared lock on the working copy. If an exclusive lock
 117 +       # already exists, downgrade it to a shared lock.
 118 +       # The lock will be dropped on program exit.
 119 +       # Note: This lock should only be taken _after_ the main wiki
 120 +       # lock.
 121 +
 122 +       # TODO
 123 +} #}}}
 124 +
 125 +sub unlockwc () { #{{{
 126 +       close WIKIWCLOCK;
 127 +} #}}}
 128 +
 129  sub loadindex () { #{{{
 130         open (IN, "$config{wikistatedir}/index") || return;
 131         while (<IN>) {
 132 </pre>
 133 """]]
 134
 135 My alternative idea, which seems simpler than all this tricky locking
 136 stuff, is to introduce a new lock file (really a flag file implemented
 137 using a lock), which tells the commit hook that the CGI is running, and
 138 makes the commit hook a NOOP.
 139
 140 * CGI takes the wikilock
 141 * CGI writes changes to WC
 142 * CGI sets wclock to disable the commit hook
 143 * CGI does *not* drop the main wikilock
 144 * CGI commit
 145 * The commit hook tries to set the wclock, fails, and becomes a noop
 146   (it may still need to send commit mails)
 147 * CGI removes wclock, thus re-enabling the commit hook
 148 * CGI updates the WC (since the commit hook didn't)
 149 * CGI renders the wiki
 150
 151 > It seems like there are two things to be concerned with: RCS commit between
 152 > disable of hook and CGI commit, or RCS commit between CGI commit and re-enable
 153 > of hook. The second case isn't a big deal if the CGI is gonna rerender
 154 > everything anyhow. --[[Ethan]]
 155
 156 I agree, and I think that the second case points to the hooks still being
 157 responsible for sending out commit mails. Everything else the CGI can do.
 158
 159 I don't believe that the first case is actually a problem: If the RCS
 160 commit does not introduce a conflict then the CGI commit's changes will be
 161 merged into the repo cleanly. OTOH, if the RCS commit does introduces a
 162 conflict then the CGI commit will fail gracefully. This is exactly what
 163 happens now if RCS commit happens while a CGI commit is in progress! Ie:
 164
 165 * cgi takes the wikilock
 166 * cgi writes change to wc
 167 * svn commit -m "conflict" (this makes a change to repo immediately, then
 168   runs the post-commit hook, which waits on the wikilock)
 169 * cgi drops wikilock
 170 * the post-commit hook from the above manual commit can now run.
 171 * cgi calls rcs_commit, which fails due to the conflict just introduced
 172
 173 The only difference to this scenario will be that the CGI will not drop the
 174 wiki lock before its commit, and that the post-commit hook will turn into a
 175 NOOP:
 176
 177 * cgi takes the wikilock
 178 * cgi writes change to wc
 179 * cgi takes the wclock
 180 * svn commit -m "conflict" (this makes a change to repo immediately, then
 181   runs the post-commit hook, which becomes a NOOP)
 182 * cgi calls rcs_commit, which fails due to the conflict just introduced
 183
 184 Actually, the only thing that scares me about this apprach a little is that
 185 we have two locks. The CGI takes them in the order (wikilock, wclock).
 186 The commit hook takes them in the order (wclock, wikilock). This is a
 187 classic potential deadlock scenario. _However_, the commit hook should
 188 close the wclock as soon as it successfully opens it, before taking the
 189 wikilock, so I think that's ok.