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

[Beginner]: Remove redundant `mMutex` hold around `mLogger.warn` in `Client::scheduleNetworkUpdate`

Open
#1,611 6 comments 0 reactions 0 assignees View on GitHub
priority: low scope: core skill: beginner status: ready for dev
Dominant language
C++
Stars
42
Forks
108
Avg merge
11h 45m
Merged PRs (30d)
2

Description

### 🐥 Beginner Friendly

This issue is a great fit for contributors who are ready to explore the Hiero C++ codebase a little more and take on slightly more independent work.

Beginner Issues often involve reading existing C++ code, understanding how different parts of the SDK fit together, and making small, thoughtful updates that follow established patterns.

The goal is to support skill growth while keeping the experience approachable, well-scoped, and enjoyable.

> [!IMPORTANT]
> ### 🐥 About Beginner Issues
>
> Beginner Issues are a great next step for contributors who feel comfortable with the basic project workflow and want to explore the codebase a little more.
>
> These issues often involve:
> - Reading existing C++ code
> - Understanding how different parts of the SDK fit together
> - Making small, thoughtful updates that follow established patterns
>
> You'll usually see Beginner Issues focused on things like:
> - Small, well-scoped improvements to existing tests
> - Narrow updates to `src` functionality (e.g. refining helpers or improving readability)
> - Documentation or comment clarity
> - Enhancements to existing examples
>
> Other types of contributions — such as brand-new features, broader system changes, or deeper technical work — are just as valuable and may use different labels.

### 👾 Description of the Task

In `Client::scheduleNetworkUpdate`, a `log4cxx`-backed `mLogger.warn(...)` call at [src/sdk/main/src/Client.cc:1222](../../src/sdk/main/src/Client.cc#L1222) is invoked while `mMutex` is held. `log4cxx`'s `Logger` is internally thread-safe (its loggers do their own locking), so the outer `mMutex` is redundant for that specific call.

PR #1533 surfaced this:

> Author confirmation: *"Logger wraps `log4cxx::Logger` which is internally thread safe, so the outer `mMutex` is technically redundant here."*
> Maintainer reply: *"We don't need to hold up the merge here, we can open up a follow-up issue to remove it."*

Holding `mMutex` longer than necessary is harmless on the happy path but slightly increases lock contention and (under pathological log-sink slowness) could prolong other waiters. This is a small cleanup, not a correctness fix.

Relevant files:

```
src/sdk/main/src/Client.cc
src/sdk/main/include/Client.h (only if a method signature needs to change — likely not)
```

### 💡 Proposed Solution

Restructure the locked region so the `mLogger.warn(...)` call happens after `mMutex` has been released. The simplest shape is to capture into local variables anything the warn message reads from member state, release the lock, then call `mLogger.warn(...)`:

```cpp
{
std::unique_lock lock(mMutex);
// ... existing work that genuinely requires the mutex ...
// capture any state needed for the log message into locals here
}
// lock released
mLogger.warn(/* use locals */);
```

If the warn message references nothing beyond literals and the captured locals, no other change is needed. If it touches additional member state, capture that too — do not re-acquire `mMutex` to read it back.

### 👩‍💻 Implementation Steps

- [ ] Open [src/sdk/main/src/Client.cc](../../src/sdk/main/src/Client.cc) and read the function around line 1222 to confirm exactly which member fields the warn message references.
- [ ] Restructure the locked block so the `mLogger.warn(...)` call sits outside the `unique_lock`'s scope. Capture any required state into locals before the lock releases.
- [ ] Confirm no other code path in `scheduleNetworkUpdate` re-relies on the lock being held across the warn call (i.e. that the post-warn work is also already either lock-free or re-acquires its own lock as needed).
- [ ] Build and run the full unit suite. Pay particular attention to `ClientUnitTests` and any test that exercises the network-update thread.
- [ ] If you can stress the bg update path locally (e.g. by running the existing concurrency-related test on a tight loop), do so — this is a deadlock-adjacent function and the cleanup should not regress the deadlock fix from PR #1533.

### ✅ Acceptance Criteria

- [ ] **Scope:** Changes are limited to `Client.cc` (and `Client.h` only if strictly necessary).
- [ ] **Behavior:** The `mLogger.warn(...)` call is invoked without `mMutex` held; the deadlock fix from PR #1533 is preserved.
- [ ] **Tests:** All existing tests pass; no new tests required (this is a refactor of an existing behavior).
- [ ] **Review:** All code review feedback addressed.

---

### 📋 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).
- This is a small, surgical change. If you find yourself wanting to also restructure the surrounding locked region, stop and split that into a separate PR — keep this one tight.
- See also the companion follow-up *"Start `Client` network update thread lazily…"* — these touch overlapping lines and can be coordinated.

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

Open src/sdk/main/src/Client.cc and inspect Client::scheduleNetworkUpdate around line 1222, especially the unique_lock scope and the state read by mLogger.warn(...). Move the warning call outside the lock while preserving the existing locked work and capturing any required state. Build and run ClientUnitTests plus the full unit suite, checking the network-update thread behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
api, networking
Issue type
Refactor
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.