I'm a little bit concerned about releasing the MVar lock in the case of an
exception. If something bad happens, not just the thread dies, but the
whole server can get stuck on an unreleased lock. And it's also slightly
more difficult to maintain.

I was thinking how we could use "bracket" or "try" and and the same time
 to release the lock before issuing the RPC call. I believe we could use
ResultT to help. Here is my idea:

handleCall qlock _ cfg (ArchiveJob jid) =
  liftM (return . showJSON . genericResult (const False) id) . runResultT $
do
    qDir <- liftIO $ queueDir
    (mcs, live, archive) <- bracketT_ (takeMVar qlock) (putMVar qlock ()) $
do
      (job, _) <- ResultT $ loadJobFromDisk qDir False jid
      guard $ jobFinalized job
      let mcs = Config.getMasterCandidates cfg
          live = liveJobFile qDir jid
          archive = archivedJobFile qDir jid
      liftIO $ renameFile live archive
      return (mcs, live, archive)
    _ <- liftIO $ executeRpcCall mcs $ RpcCallJobqueueRename [(live,
archive)]
    return True
  where
    bracketT_ :: IO a -> IO b -> ResultT e IO c -> ResultT e IO c
    bracketT_ start end = ResultT . bracket_ start end . runResultT

(I haven't tested the code, I just compiled it.)


I have been thinking lately how to ease error handling with multiple nested
Results etc., and I made a few helper functions that helped me to use
ResultT more easily, primarily for converting different types of errors
(like to get rid of the not-very-nice genericResult above). I tried to
refactor a few code pieces to see how well it works. But I haven't pushed
them yet, hopefully very soon.

  Best,
  Petr



On Wed, Jan 22, 2014 at 3:00 PM, Klaus Aehlig <[email protected]> wrote:

> With luxid taking over the tasks of masterd, archiving
> jobs also belongs to its responsibilities. As archiving
> a job affects the global state of the job queue, synchronise
> over the queue lock.
>
> Signed-off-by: Klaus Aehlig <[email protected]>
> ---
>  src/Ganeti/Query/Server.hs | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/src/Ganeti/Query/Server.hs b/src/Ganeti/Query/Server.hs
> index ae8fcc5..8d48c95 100644
> --- a/src/Ganeti/Query/Server.hs
> +++ b/src/Ganeti/Query/Server.hs
> @@ -334,6 +334,26 @@ handleCall _ qstat  cfg (CancelJob jid) = do
>        cancelJob jid
>      Bad s -> return . Ok . showJSON $ (False, s)
>
> +handleCall qlock _ cfg (ArchiveJob jid) = do
> +  let archiveFailed = putMVar qlock  () >> (return . Ok $ showJSON False)
> +                      :: IO (ErrorResult JSValue)
> +  qDir <- queueDir
> +  takeMVar qlock
> +  result <- loadJobFromDisk qDir False jid
> +  case result of
> +    Bad _ -> archiveFailed
> +    Ok (job, _) -> if jobFinalized job
> +                     then do
> +                       let mcs = Config.getMasterCandidates cfg
> +                           live = liveJobFile qDir jid
> +                           archive = archivedJobFile qDir jid
> +                       renameFile live archive
> +                       putMVar qlock ()
> +                       _ <- executeRpcCall mcs
> +                              $ RpcCallJobqueueRename [(live, archive)]
> +                       return . Ok $ showJSON True
> +                     else archiveFailed
> +
>  handleCall _ _ _ op =
>    return . Bad $
>      GenericError ("Luxi call '" ++ strOfOp op ++ "' not implemented")
> --
> 1.8.5.3
>
>

Reply via email to