hiero-ledger / hiero-ledger/hiero-sdk-cpp

[Intermediate]: Start `Client` network update thread lazily after both networks are configured

Open
#1,610 0 comments 0 reactions 0 assignees View on GitHub
priority: low priority: medium scope: core scope: network status: ready for dev
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.