hiero-ledger / hiero-ledger/hiero-sdk-cpp
[Intermediate]: Start `Client` network update thread lazily after both networks are configured
- Dominant language
- C++
- Stars
- 42
- Forks
- 108
- Avg merge
- 11h 45m
- Merged PRs (30d)
- 2
Description
### 🧩 Intermediate Friendly
This issue is a good fit for contributors who are already familiar with the Hiero C++ SDK and feel comfortable navigating the codebase.
Intermediate Issues often involve:
- Exploring existing implementations
- Understanding how different components work together
- Making thoughtful changes that follow established patterns
The goal is to support deeper problem-solving while keeping the task clear, focused, and enjoyable to work on.
> [!IMPORTANT]
> ### 🧭 About Intermediate Issues
>
> Intermediate Issues are a great next step for contributors who enjoy digging into the codebase and reasoning about how things work.
>
> These issues often:
> - Involve multiple related files or components
> - Encourage investigation and understanding of existing behavior
> - Leave room for thoughtful implementation choices
> - Stay focused on a clearly defined goal
>
> Other kinds of contributions — from beginner-friendly tasks to large system-level changes — are just as valuable and use different labels.
### 👾 Description of the Task
`Client::scheduleNetworkUpdate` runs in a background thread and currently uses a `continue` null-network guard at [src/sdk/main/src/Client.cc:1185](../../src/sdk/main/src/Client.cc#L1185): if the consensus or mirror network is not yet configured, the loop wakes up, sees the missing network, and goes back to sleep.
PR #1533 (which fixed a deadlock in the same code path) raised this as a design observation:
> Maintainer comment on PR #1533: *"A more principled design would be to start the update cycle only once both a mirror network and a consensus network are set… Worth a follow-up issue if you're interested."*
> Author reply: *"I can open a follow-up issue to track it."*
> Maintainer reply: *"Sounds good, please do so."*
The follow-up was never filed. The skip-loop still works correctly — this is a design improvement, not a correctness fix — so the priority is low. But spinning a thread that immediately re-sleeps every cycle until both networks are wired is wasteful, and the skip-loop obscures the actual lifecycle of the update thread.
Relevant files:
```
src/sdk/main/include/Client.h
src/sdk/main/src/Client.cc
src/sdk/tests/unit/ClientUnitTests.cc
```
### 💡 Proposed Approach
Two reasonable shapes:
1. **Lazy start.** Don't spawn the network update thread inside the `Client` constructor. Instead, start it the first time _both_ a consensus network and a mirror network have been configured (e.g. inside the setter that completes the pair, gated by an "already started" flag). Stop the thread only at destruction, as today.
2. **Condition-variable wait.** Keep the constructor-time spawn, but have the thread `wait()` on a `std::condition_variable` until both networks are present, instead of sleeping-and-rechecking. The setter that completes the pair `notify_one()`s the cv.
Approach (1) is simpler and matches the maintainer's suggested wording ("start the update cycle only once both… are set"). Approach (2) keeps the thread-lifecycle parallel between configured and unconfigured `Client` instances at the cost of a slightly more involved sync primitive.
Either works. Pick the one whose blast radius is smaller given how `Client` is constructed today, and explain the choice in the PR description.
Make sure the chosen approach plays nicely with:
- `setConsensusNetwork(...)` / `setMirrorNetwork(...)` (or whichever setters establish each).
- The destructor's join semantics for the update thread.
- Existing unit tests that construct a `Client` with neither, one, or both networks set.
### 👩💻 Implementation Steps
- [ ] Read [src/sdk/main/src/Client.cc](../../src/sdk/main/src/Client.cc) around lines 1150–1230 (the deadlock-fix code path) and the `Client` constructor / setter surface.
- [ ] Pick approach (1) lazy-start or (2) condition-variable wait. Note your reasoning for the PR description.
- [ ] Implement the change. The `continue` null-network guard at line 1185 should be removed (its purpose disappears under either approach).
- [ ] Review the destructor: confirm the update thread is still joined cleanly when (a) the `Client` was destroyed before any network was set, (b) one network was set, (c) both networks were set.
- [ ] Add a unit test (or extend an existing one) covering the lifecycle: construct a `Client` with no networks set, assert no update thread is running (or that it is in a wait state); set one network, then both; assert the update cycle is now active.
- [ ] Run the full unit suite to confirm no regressions: `ctest -j 6 -C Debug --test-dir build/linux-x64-debug -R Client`.
### ✅ Acceptance Criteria
- [ ] The `continue` null-network guard at the current `src/sdk/main/src/Client.cc:1185` is removed.
- [ ] The network update thread does not run until both consensus and mirror networks are configured.
- [ ] All existing unit tests pass; at least one new test covers the lifecycle change.
- [ ] No public API surface changes (the setter signatures and destructor behavior are unchanged).
---
### 📋 Step-by-Step Contribution Guide
To help keep contributions consistent and easy to review, we recommend following these steps:
- [ ] Comment `/assign` to request the issue
- [ ] Wait for assignment
- [ ] Fork the repository and create a branch
- [ ] Set up the project using the instructions in `README.md`
- [ ] Make the requested changes
- [ ] Sign each commit using `-s -S`
- [ ] Push your branch and open a pull request
Read [Workflow Guide](https://github.com/hiero-ledger/hiero-sdk-cpp/blob/main/docs/training/workflow.md) for step-by-step workflow guidance.
Read [README.md](https://github.com/hiero-ledger/hiero-sdk-cpp/blob/main/README.md) for setup instructions.
❗ Pull requests **cannot be merged** without `S` and `s` signed commits.
See the [Signing Guide](https://github.com/hiero-ledger/hiero-sdk-cpp/blob/main/docs/training/signing.md).
### 🤔 Additional Information
- Originating PR: [#1533](https://github.com/hiero-ledger/hiero-sdk-cpp/pull/1533) — fixed the deadlock; this issue addresses the design improvement raised in the same review.
- Companion follow-up to consider together: removing the redundant `mMutex` hold around `mLogger.warn` in the same function (see the *"Remove redundant `mMutex` hold around `mLogger.warn`"* issue). The two changes touch overlapping lines and may be ergonomic to ship in sequence.
If you have questions while working on this issue, feel free to ask! [Hiero-SDK-C++ Discord](https://discord.com/channels/905194001349627914/1337424839761465364)
Contributor guide
Research direction
Read src/sdk/main/src/Client.cc around lines 1150–1230, then inspect src/sdk/main/include/Client.h and the Client constructor and network setters. Compare the lazy-start and condition-variable options, and review src/sdk/tests/unit/ClientUnitTests.cc before running ctest -j 6 -C Debug --test-dir build/linux-x64-debug -R Client. Done means the update thread waits until both networks are configured, lifecycle tests cover no, one, and both networks, and the null-network guard is removed without changing the public API.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- distributed-systems, networking
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 55/100