[darcs-users] darcs patch: Properly handle errors from request_url.

[Eric Kow]

Hi,

On Fri, Apr 10, 2009 at 19:01:21 +0400, Dmitry Kurochkin wrote:
> Patch description says it all. Hopefully this would resolve issues
> 1368 and 1420. Or at least get us forward.

Thanks for the ping, and sorry for the delay.  I didn't fully understand
this patch, but it looks like something I can apply.  In the worst case,
we can roll it back :-)

I think we may need a little more documentation in the URL module, for
example, explaining us what the various fields in the UrlState record
are for.  If Sigbjorne does manage to extend his curl code, I'm guessing
that we'll still need to have this module around, at least to manage our
own queuing stuff, right?  If so, it'd be good to document it for
posterity.

> Fri Apr 10 18:51:26 MSD 2009  Dmitry Kurochkin <dmitry.kurochkin at gmail.com>
>   * Properly handle errors from request_url.
>   If request_url returns an error it is not enough just to print
>   a debug message. That would lead to potential errors when
>   wait_next_url is called for URLs that were not (successfully)
>   requested.
>   
>   We should correctly update UrlState and put error into notification
>   variable.

Properly handle errors from request_url.
----------------------------------------
This patch modifies the function URL.checkWaitToStart.

My understanding of checkWaitToStart is that it is pipelining-related:
it pulls as many URLs off the queue as we have slots per HTTP request
(arbitrarily? defined as 100) and some other piece of code sends them
off in one go.  checkWaitToStart is recursive; it processes one URL at a
time, making the actual request eg via curl, incrementing a counter
representing the number of slots we have used; and then calls itself
recursively until we've used up all of our slots.

(*) I wonder if we have/need some mechanism to force a request even if
    we're not up to our limit

> ] hunk ./src/URL.hs 187
>      case readQ w of
>        Just (u,rest) -> do
>          case Map.lookup u (inProgress st) of
> -          Just (f, _, c, _) -> do
> -            put $ st { waitToStart = rest
> -                     , pipeLength = l + 1 }
> +          Just (f, _, c, v) -> do
>              dbg ("URL.request_url ("++u++"\n"++
>                   "              -> "++f++")")
>              let f_new = f++"-new_"++randomJunk st

I'm going to tweak the whitespace and interleaving below for what I hope
to be added clarity

> -            if null err
> -               then do atexit $ removeFileMayNotExist f_new
> -                       debugMessage "URL.request_url succeeded"

> +            if null err
> +               then do dbg "URL.request_url succeeded"
> +                       liftIO $ atexit (removeFileMayNotExist f_new)
> +                       put $ st { waitToStart = rest
> +                                , pipeLength = l + 1 }

Basically nothing here has changed (the 'put' used to be executed in
any case; now it's only in the case where we succeed)

> -               else do removeFileMayNotExist f_new
> -                       debugMessage $ "Failed to start download URL "++u++": "++err
> +               else do dbg $ "Failed to start download URL "++u++": "++err
> +                       liftIO $ do removeFileMayNotExist f_new
> +                                   putMVar v err
> +                       put $ st { waitToStart = rest }

The two changes here seem to be the putMVar v err, and not adding to
pipeLength.

First the MVar: this associates the URL with an error message.  I don't
really understand how this MVar is used exactly, though, although I see
it is passed around between urlNotifications, UrlRequest and inProgress.
My guess is that waitUrl is what gets called when we finally decide we
really want the URL.  This waits until we've actually tried to fetch the
URL and then bails out [Darcs.Global.debugFail] if anything went wrong.

Second: not adding to pipeLength.  I'm guessing this modification is not
entirely necessary.  It's just saying that since we didn't manage to
start downloading this URL, we should leave ourselves the room for one
more request.  I have a feeling that I'm missing something here, though.
Why bother?  Won't this part of the code ultimately lead to a failure
anyway?

PS. I didn't really know what an MVar is until I tried to review this
    patch.  Yay for learning Haskell through darcs patch review!

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 194 bytes
Desc: not available
URL: <http://lists.osuosl.org/pipermail/darcs-users/attachments/20090506/baa62802/attachment.pgp>