Don't combine distribution if count == 0

[mitchellwrosen]

This is one possible solution to #24.

[23Skidoo]

I'm not very familiar with this part of ekg, would be nice if @tibbe could take a look.

[mitchellwrosen]

Bump, thoughts @tibbe? :)

[23Skidoo]

@tibbe thinks it's good to go, merging.

[23Skidoo]

Merged, thanks!

[pepeiborra]

I don't know how, but this change led to a 100% reproducible lock-up in one of our EKG enabled services at work (Strats SCB). This is using 8.4.4. We forked ekg-core while preparing for an upgrade to 8.6.2. Unfortunately I cannot provide a repro for obvious reasons.

[mitchellwrosen]

Ouch. I'm sorry about that @pepeiborra. Can you provide any more information?

[23Skidoo]

Reverted, released a new bugfix version.

[pepeiborra]

I would love to @mitchellwrosen but I can't share any concrete artefacts from work. The only details I have:

• It was a hard lock-up, the Haskell scheduler stopped generating events (I was running with -N1). This was observable using the debug runtime and using +RTS -De -Ds iirc. This suggested it was an unsafe foreign call stealing the thread away from the Haskell scheduler.
• I found the culprit only by bisecting the packages in the diff between nightly-2018-11-xx and lts-12.18

[adamse]

Possible diagnosis:

The quick exit path in hs_distrib_add_n added here doesn't release the lock so any subsequent calls to hs_distrib_add_n with the same b param will wait forever in hs_lock(&b->lock).

[nh2]

  hs_lock(&b->lock);

+  if (!b->count) {
+    return;
+  }

Returning after taking a lock, this cannot work.

[mitchellwrosen]

@nh2 Right, that's the obvious bug. I apologize :(

[nh2]

@mitchellwrosen Can happen! It seems ekg needs better tests if a simple change like this can take down Standard Chartered's upgrade at runtime.

EDIT: I stand corrected. ekg has no tests at all.