HALON-823: fix sporadic notification failures

[1468ca0b-2a64-4fb4-8e52-ea5806644b4c]

Created by: andriytk

The problem was in ha_connect handler where the all the link's pending notification delivery tags were flushed out. If ha_connect would come between notification sending and delivery callback, such notification would never be confirmed to be delivered and hence would fail.

[1468ca0b-2a64-4fb4-8e52-ea5806644b4c]

Created by: andriytk

Maybe the confusion comes from the fact that variable tags here actually means map of links each having the single tag in its map value? Anyway, let me know if you will have some ideas to simplify it.

[1468ca0b-2a64-4fb4-8e52-ea5806644b4c]

Created by: andriytk

Nice! Will change it.

[1468ca0b-2a64-4fb4-8e52-ea5806644b4c]

Created by: vvv

OK, whatever. I still have difficulties with reading this line (due to ifor), but let it be.

[1468ca0b-2a64-4fb4-8e52-ea5806644b4c]

Created by: vvv

If the purpose of length mv is to find out whether mv is empty, this better be done explicitly:

" has_callback=" ++ show (not $ Map.null mv)

With this implementation comment is not needed.

[1468ca0b-2a64-4fb4-8e52-ea5806644b4c]

Created by: vvv

Because its name is self-descriptive. Fewer aliases to remember.

[1468ca0b-2a64-4fb4-8e52-ea5806644b4c]

Created by: vvv

Still, head is quite dangerous. We should strive to avoid partial functions.

If you are 100% sure that tags is a singleton, at least add a comment explaining why this is so. Normally such convictions are best described with types or helper functions.

Speaking of helper functions. Have you heard of safe package? It exports headDef function, which is “head with default value”.

headDef :: a -> [a] -> a

No, I don't propose that we add safe to the list of Halon dependencies. I just thought that this library is worth mentioning.

[1468ca0b-2a64-4fb4-8e52-ea5806644b4c]

Created by: andriytk

toAscList is just an alias to assocs. Why?

[1468ca0b-2a64-4fb4-8e52-ea5806644b4c]

Created by: andriytk

Added.

[1468ca0b-2a64-4fb4-8e52-ea5806644b4c]

Created by: andriytk

Right, it follows from the code. (Notice singleton above.)

[1468ca0b-2a64-4fb4-8e52-ea5806644b4c]

Created by: andriytk

Ok.

[1468ca0b-2a64-4fb4-8e52-ea5806644b4c]

Created by: vvv

Well, it didn't follow from the types.

[1468ca0b-2a64-4fb4-8e52-ea5806644b4c]

Created by: andriytk

Right, agree. Thanks!

[1468ca0b-2a64-4fb4-8e52-ea5806644b4c]

Created by: andriytk

As I said, there is only one tag here per link always!

[1468ca0b-2a64-4fb4-8e52-ea5806644b4c]

Created by: vvv

There is more direct way:

return $ Map.insertWith Map.union hl Map.empty links

[1468ca0b-2a64-4fb4-8e52-ea5806644b4c]

Created by: vvv

Can you add this comment somewhere in the code? This information seems to be useful.

[1468ca0b-2a64-4fb4-8e52-ea5806644b4c]

Created by: vvv

fmap Map.keys <$> Map.toAscList tags

has type [(HA.HALink, [Word64])]. This type is Showable.

I don't see the purpose of head. (0) It will cause runtime error if tags is empty. (1) It leaves the first Word64 value (tag), ignoring the rest. What's so special about that first tag?

Suggestion:

      log $ "notifyMero: epoch=" ++ show epoch ++ " [(link, tags)]="
          ++ show (fmap Map.keys <$> Map.assocs tags)

[1468ca0b-2a64-4fb4-8e52-ea5806644b4c]

Created by: andriytk

It probably can, but I'm not sure how. head is used because there is only one tag in the Map here and we don't want to see the list of only one element. We rather want to see this element by itself.

[1468ca0b-2a64-4fb4-8e52-ea5806644b4c]

Created by: andriytk

The number of callbacks per tag (if found) is at most one. So we actually check here if there was any tag found in the map of callbacks.

[1468ca0b-2a64-4fb4-8e52-ea5806644b4c]

Created by: vvv

I cannot parse this expression. Can it be simplified?

Also, why head is needed here?

[1468ca0b-2a64-4fb4-8e52-ea5806644b4c]

Created by: vvv

Why number of elements is labeled as cb?

[1468ca0b-2a64-4fb4-8e52-ea5806644b4c]

Created by: vvv

Not needed. See below.

[1468ca0b-2a64-4fb4-8e52-ea5806644b4c]

Created by: vvv

This expression doesn't look good.

The type of mv is

mv :: Maybe (Map Word64 Callback)

You want to log all keys of the map.

Accessing the keys:

Map.keys <$> mv :: Maybe [Word64]

An expression of type Maybe [Word64] is Showable.

You can either

        log $ "ha_process_event_set: link=" ++ show hlink
           ++ " tags=" ++ show (Map.keys <$> mv)

or

        let mtags = Map.keys <$> mv
        log $ "ha_process_event_set: link=" ++ show hlink
           ++ maybe "" ((" tags=" ++) . show) mtags