Closed fcecin closed 2 months ago
Set this branch to DRAFT again (WIP) because I will be adding more features to it.
Namely:
I will also try to write some test coverage to obtain that satisfying green checkmark from Sonarqube.
EDIT: disabling command-line args is not obvious, because the Options "singleton" does not look like a singleton in the process sense, but in the blockchain object sense. in unit testing you can have multiple Options objects, so you still need to be able to load and set process-wide options that apply to everything. For example, we don't want custom log levels or threading parameters for each simulated blockchain in unit testing.
Failed conditions
38.4% Coverage on New Code (required ≥ 80%)
Unfortunately:
ManagerNormal::handleMessage():
the default:
label is "uncovered" because we generate an exception inside of Message::type()
that is invoked as the case
statement. Removing the defensive default:
label just to satisfy Sonarqube is arguably a bad idea.I don't think we need to spend time refining code coverage of things like exception handlers. Most of them are there preventively.
This PR in a nutshell:
Message
object is handled in the same net thread that runs the P2P engine, instead of being offloaded to a separate "incoming Message processing" thread pool (the oldManagerBase::threadPool_
). If there is need to offload particular tasks from net threads, this is done in the Message handler itself, and whatever higher-level thread pool(s) do this, they are of no concern to the P2P/Net componentDetailed:
NOTE: I reviewed Sonarqube's test coverage complaints and I think there's nothing relevant there.