fix(blockexchange): ensures futures are asyncSpawned

[emizzle]

• asyncSpawn futures (that raise []) in advertiser, discovery, and engine modules
• prevents failures in the futures from silently failing
• replaces engine.blockexcTasks with TrackedFutures, which tracks and cancels long-running futures

[benbierens]

I see how track() works, but I don't recommend this. If I look for the usage of the "trackedFutures" field in the engine.nim, all I find is the creation and cancel-call. I'd much prefer the connection between the field and track() to be explicit at the point of use. So a small change like
let fut = b.processQueueLoop().track(b.trackedFutures)

What do you think?

[gmega]

I confess I'm always weirded out when I see this. My simple brain parses that the Future returned by processQueuedLoop is tracking something, when in fact there's this unexpected inversion and it's actually b.trackedFutures that is tracking b.processQueueLoop.

[emizzle]

Sure, the API could use some work, but that can be handled in another PR and applied across the codebase.

[emizzle]

There are reasons the API ended up where we are now, but those reasons are being removed from the codebase, so we'll have some freedom to change the API (not in this PR).

Something like this would be better?

let fut = b.processQueueLoop()
b.trackedFutures.track(fut)
asyncSpawn fut

[marcinczenko]

Looking just at the code:

let fut = b.processQueueLoop().track(b)

it is not evident what it does. To understand you have to check the return type of processQueueLoop which apparently has ability to “track” objects of the type corresponding to b. The reader really needs to dissect the code to understand.

The proposed alternative is way easier to understand. You could even just say b.trackedFutures.add(fut) or maybe even instead of trackedFutures something like a type FutureTracker and then b.futureTracker.add(fut) or maybe there could be a special type of a “TrackedFuture” which would take care of itself magically (I do not know if that would work here - I did not dive into details)…

[emizzle]

maybe there could be a special type of a “TrackedFuture” which would take care of itself magically

There are definitely ways to do this, however I'm not a fan of magic things in most cases because they tend to hide what's happening and make things harder to reason, harder to debug, and harder to add new features (even though it's usually intended to make things easier to implement, however it's rarely the case).

[marcinczenko]

When saying "magic" I mean more something in the style of RAII (Resource Acquisition is Initialization) common in C++ and less in the style of Java/C#/Spring like fancy annotations.

[emizzle]

PR here: #1046

[gmega]

Good catch. Btw, we should probably plan to drop the v3 compat mode from Chronos as it'll then start warning about those all over.

[gmega]

Nice improvement, and good idea to reuse the TrackedFutures abstraction that we already have in place of seq[Future].