[darcs-devel] [patch1373] refactored Darcs.Repository (and 1 more)

[Ganesh Sittampalam]

Ganesh Sittampalam <ganesh at earth.li> added the comment:

Hi Ben,

Apologies for the delay in reviewing this (as always :-()

On 23/06/2015 00:45, Ben Franksen wrote:

> I am still unsure what the intention of the original code was w.r.t. the
> starting of two parallel threads. I am certain I did not misrepresent
> what the code actually did do, namely starting two threads that race for
> completion, abandoning (now: canceling) the non-yet-complete one. But is
> this really what should happen? One thread fetches all the patches the
> function gets as a parameter. The other thread fetches the patches
> listed in "meta-filelist-inventories". Are the passed patches garuanteed
> to be a subset of what's listed in meta-filelist-inventories? (This is
> absolutely not obvious and a small comment explaing the author's
> intention when writing this rather tricky code would have made things so
> much easier to understand!) Another question is why the thread that
> fetches the passed patches is started only when the meta-filelist has
> been written and not earlier. I suspect this was an artefact of the old
> implementation and could be changed now.

As far as I understand the old code, the pattern was to make one or more
calls to 'forkWithControlMVar' within the scope of a single
'withControlMVar' call:

> withControlMVar :: (MVar () -> IO ()) -> IO ()
> withControlMVar f = do
>   mv <- newMVar ()
>   f mv
>   takeMVar mv

> forkWithControlMVar :: MVar () -> IO () -> IO ()
> forkWithControlMVar mv f = do
>   takeMVar mv
>   _ <- forkIO $ finally f (putMVar mv ())
>   return ()

Don't the MVars ensure that the individual actions passed to
'forkWithControlMVar' are run in serial, and that the 'withControlMVar'
doesn't finish until the final forked action has completed? Initially
the MVar is full. When the first forkWithControlMVar runs, the MVar is
taken and the action is forked off. The second forkWithControlMVar will
have to wait for the first action to finish and call putMVar, before it
can do anything.

As I understand it your replacement code will run the actions in
parallel and then cancel the others once one completes.

As far as I can tell, in practice the number of actions that are run
depends on whether the code was in the 'basic' or the 'patches' call
path, and on how many entries were found in the tar file with a name
starting 'meta-'. I guess the expectation is that there would only be
one 'meta-' entry in the tar file, otherwise things could get a bit
confused. Under that assumption, there'd be one action in the 'basic'
call path used for reading 'pristine', and two actions in the 'patches'
call path used for reading 'inventories'.

Cheers,

Ganesh

----------
title: resolve issue2400 -> refactored Darcs.Repository (and 1 more)

__________________________________
Darcs bug tracker <bugs at darcs.net>
<http://bugs.darcs.net/patch1373>
__________________________________