Code Review

[svenkeidel]

Hi @wpmp,

I reviewed the code base and have the following recommendations.

1. Use of MVar's: Put all your state into one MVar instead of multiples, else you run into deadlocks. Also you should avoid using readMVar and putMVar because they always have to appear in pairs. Instead try to use modifyMVar:

   onRegisterMessage :: Z.Sender a => RQ.RegisterServiceRequest -> Socket a -> MVar (Sockets,SocketPool,Broker) -> IO()
   onRegisterMessage register regSocket var = modifyMVar_ var $ \(socket,pool,broker) ->  do
   let serviceID = RQ.serviceID register
   let server = B.Server (RQ.product register) (RQ.language register)
   case List.length $ B.portPool broker of
    0 -> do
      putStrLn $ unwords ["register", T.unpack serviceID, "failed: no free ports"]
      Z.send regSocket [] (BS.concat $ BSL.toChunks (A.encode (RS.RegisterServiceResponse "failed: no free ports" Nothing)))
    _ -> do
      ...

2. Use of Maybe: fromJust is a partial function and should be avoided. If you can provide a meaning default value when a Maybe turns out to be Nothing use fromMaybe meaningfullDefault maybeValue:

   Another alternative is using the Monad instance of Maybe. It allows you to short-circuit a whole computation if one maybe values is nothing:

   lookupTwoKeys k1 k2 m = do
   v1 <- M.lookup k1 m
   v2 <- M.lookup k2 m
   return (v1,v2)

   If you have several Maybe values of the same type and you just want to get one of the values that is Just, you can use the Monoid instance of Maybe:

   getOneOf :: Maybe a -> Maybe a -> Maybe a -> a -> a
   getOneOf maybe1 maybe2 maybe3 default = fromMaybe default (maybe1 <> maybe2 <> maybe3)

3. Eliminate redundant code: broker/Main.hs contains 6 times the following code:

   Z.send regSocket [] (BS.concat $ BSL.toChunks (A.encode (RS.RegisterServiceResponse "some message" Nothing)))

   You should introduce a new function for this piece of code.

If you have further questions contact me and we can discuss the issues in person.

Best, Sven

[wpmp]

Hi @svenkeidel ,

thank you very much for your helpful advices! I will try to put all my state in one MVar. I'm aware of the of Maybe and the redundant code things. Code cleanup is on my list and will be done soon.

Best regards, Wulf

[svenkeidel]

@wpmp, you asked how to dispatch the different messages in the code below. An easy solution would be to just reserve different ports for the different message types. What do you think?

    let maybeVersionMsg = (A.decodeStrict rawMsg) :: Maybe VersionMessage
    let maybeDiscoverMsg = (A.decodeStrict rawMsg) :: Maybe DiscoverRequest
    let maybeConfigMsg = (A.decodeStrict rawMsg) :: Maybe ConfigurationMessage
    case maybeVersionMsg of
      Just msg -> ...
      Nothing -> ...
    case maybeDiscoverMsg of
      Just msg -> ...
      Nothing -> ...
    case maybeConfigMsg of
      Just msg -> ...
      Nothing -> ...

[wpmp]

@svenkeidel, this would be a possibility, however I think that would make implementation of sources in clients more complex. I would probably leave it like this for now. Whats your opinion?

[svenkeidel]

Can you give an example of what would be more difficult this way?

[wpmp]

Nevermind, i mixed up some things. I thought about it again and I think this might also be helpful for performance as configuration and discover messages can't block version and product messages, right?

[svenkeidel]

yes