♻️ Refactor `NetMQTransport`'s initialization

[greymistcube]

Related to #1612, #1934. Some preparatory work to streamline uses of ITransport and Swarm<T>.

This is mostly to move up ITransport's binding phase more in line with the rest of the Libplanet.Net's (seemingly) intended design. 🙄

There are some problematic uses of async and CancellationToken handling here and there, which should be dealt with shortly. 😐

Hopefully with this, we might be ready to combine Peer and BoundPeer into a single class.

[longfin]

I'm somewhat skeptical of the need to force sync for ITransport (and Swarm<T>) creation. of course C# doesn't support async for constructor so that we need to additional async factory method. but in other hands, async factory method gives the user the freedom to choose how they want to handle the wait.

It may feel like this kind of freedom is at odds with usability, but unless we're making the decision to eliminate delays in the creation of these objects, at least we need to provide a way to deal with them by default. and if necessary, it may be more appropriate to provide a framework-level helper such as DI extensions or UniLibplanet instead of a synchronous constructor.

That could be an entirely different story, but it's also a topic worth considering if we really need to get rid of the "uninitialized state". At least I agree with the problem that there is something more confusing and awkward about it at this point, but considering many ecosystems around C#/.NET(like DI) that already assumes, we have options as belows

• (as suggested in this PR) force to wait on synchornous APIs.
  • or, shift the responsibility of initializing it to the user. [^1]
• admit "unintilalzed state" 😇.

In that case, I can't keep my agreement about refusing "unintilalzed state" easily because flexibility about blocking is also matter IMHO.😢

[^1]: when using async factory method in DI, it will be implemented with this pattern.

cc @planetarium/libplanet

[greymistcube]

I agree, in general, this is a bad practice. My opinion is that, however, this is causing way more trouble than it should. Ideally, async static factory method for creating an ITransport (with more detailed specification) and handing the responsibility over to the user would be better. This, however, is no small task. Design-wise, I also agree having a user create an "initialized" ITransport and handing it over to Swarm<T> would make a lot more sense, but there are too many things to consider to "simply" change to a better design (which is why this issue keeps getting kicked down the road):

• Separating out Initialize() from StartAsync()
• Moving up Initialize() to earlier stage
• Enforcing a call to Initialize() outside Swarm<T>
• Creation of new Swarm<T> being part of synchronous code (at least for 9c-headless)
• Options is still a mess 😐

I really didn't want to (nor do I have the time to) open a new PR with 1,000+ lines of code change.

As for admitting an "uninitialized state", I think creation of TurnClient is "low-level" enough not to expose it to an end-user. I'm also not happy with how it looks at the moment.

I'm really not happy with how it looks at the moment, but I want to keep at it really, as now NetMQTransport is getting also used for PBFT gossip. This is just a really awkward compromise while trying to unentangle Swarm<T> and ITransport. There is really no reason why Swarm<T> should be responsible for initializing an ITransport. Also Swarm<T>.Initialize() was a no go for me. So I forcefully (and awkwardly) stuck it in Swarm<T>(). 🙄

Perhaps, this is too sloppy I suppose. I'll try to incorporate some design-choices as to hint more at what the end goal is. 😗

[pull-request-quantifier-deprecated[bot]]

This PR has 254 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

---

Quantification details

Label      : Large
Size       : +136 -118
Percentile : 65.4%

Total files changed: 11

Change summary by file extension:
.md : +13 -0
.cs : +123 -118

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a balance between between PR complexity and PR review overhead. PRs within the optimal size (typical small, or medium sized PRs) mean:

• Fast and predictable releases to production:
  • Optimal size changes are more likely to be reviewed faster with fewer iterations.
  • Similarity in low PR complexity drives similar review times.
• Review quality is likely higher as complexity is lower:
  • Bugs are more likely to be detected.
  • Code inconsistencies are more likely to be detected.
• Knowledge sharing is improved within the participants:
  • Small portions can be assimilated better.
• Better engineering practices are exercised:
  • Solving big problems by dividing them in well contained, smaller problems.
  • Exercising separation of concerns within the code changes.

What can I do to optimize my changes

• Use the PullRequestQuantifier to quantify your PR accurately
  • Create a context profile for your repo using the context generator
  • Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
  • Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
  • Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
• Change your engineering behaviors
  • For PRs that fall outside of the desired spectrum, review the details and check if:
    • Your PR could be split in smaller, self-contained PRs instead
    • Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

• One line was added: +1 -0
• One line was deleted: +0 -1
• One line was modified: +1 -1 (git diff doesn't know about modified, it will interpret that line like one addition plus one deletion)
• Change percentiles: Change characteristics (addition, deletion, modification) of this PR in relation to all other PRs within the repository.

---

Was this comment helpful? :thumbsup:  :ok_hand:  :thumbsdown: (Email) Customize PullRequestQuantifier for this repository.

[greymistcube]

@longfin So as discussed, I've tried to keep things async as much as possible. 😶

We still have ugly and awkward GetAwaiter().GetResult() stuck in Swarm<T>() constructor, but this should at least make it easier to extract it out from Swarm<T>() later down the road (although a proper handling shared PrivateKey and some other stuff between Swarm<T> and ITransport might be little tricky).