fix(contrib/nacos): move to the upstream commit that fixes the RpcClient data race by lingcoder · Pull Request #4856 · gogf/gf

lingcoder · 2026-08-18T11:46:35Z

Summary

CI fails at random in contrib/registry/nacos with a data race that lives entirely inside the SDK:

WARNING: DATA RACE
Read at 0x00c00037e848 by goroutine 22:
  (*RpcClient).notifyConnectionEvent()  rpc_client.go:487
Previous write at 0x00c00037e848 by goroutine 23:
  (*RpcClient).reconnect()              rpc_client.go:449

No gf frame takes part in the racing accesses — this repository only appears at the call entry, nacos.go:93 calling clients.NewNamingClient(). Reported as #4846.

Why pin a commit

In v2.3.5 the field is plain:

currentConnection           IConnection      // v2.3.5

Upstream made it atomic, which is exactly the right fix:

currentConnection           atomic.Value     // master, stores IConnection

That fix is not in any release. v2.3.5 was published 2025-10-26 and master is 11 commits ahead of it, so pinning the commit is the only way to pick it up right now. The go.mod entries resolve to v2.3.6-0.20260713020125-4ba94455701e, and should be moved back to a tag once upstream publishes one.

contrib/config/nacos was still on v2.3.3, two releases behind contrib/registry/nacos. Both modules now reference the same version, so the two nacos modules cannot drift apart again.

Verification

Both modules build and go vet clean.
TestRegistry and TestWatch fail identically before and after the change on a machine with no nacos server running, so the upgrade introduces no regression. Verified against a clean master worktree.
The race itself needs a reconnect to trigger, so CI is the real test.

closes #4846

…ent race nacos-sdk-go v2.3.5 stores RpcClient.currentConnection as a plain field while reconnect() writes it and notifyConnectionEvent()/Request() read it, so -race reports a data race whenever a reconnect happens. CI hits it at random and, with fail-fast on, it takes the whole matrix down. Upstream fixed it by making the field an atomic.Value, but that fix has not been released: v2.3.5 is from 2025-10-26 and master is 11 commits ahead. Pinning the commit is the only way to get it today. contrib/config/nacos was still on v2.3.3; both modules now use the same version.

…run (#4857) ## The flake `TestApollo` fails at random on pull requests that touch neither Apollo nor `contrib/` — recently on #4844, #4851 and #4854. ``` [ASSERT] EXPECT == :8000 --- FAIL: TestApollo (10.01s) ``` What passes right before it is the interesting part: ```go adapter, err := apollo.New(ctx, apollo.Config{...}) t.AssertNil(err) // passes t.Assert(config.Available(ctx), true) // passes v, err := config.Get(ctx, `server.address`) t.AssertNil(err) // passes t.Assert(v.String(), ":8000") // fails, the value is empty ``` The client connects and the server answers; the key simply is not there yet. `loads/apollo-quick-start` boots MySQL, imports its seed data, then starts the config service — a `running` container does not mean the configuration is queryable. The adapter cannot report this either: ```go return c.value.Val().(*gjson.Json).Get(pattern).Val(), nil ``` A missing key yields `nil` from `gjson`, and there is no error to return, so an empty value and a genuinely absent key look identical to the caller. ## The fix Nothing waited for the data. The compose file declares no `healthcheck` (`grep -c healthcheck` returns 0) and the workflow moved straight on after `up -d`. Services declared under `services:` in the workflow get the runner built-in health waiting; compose-started ones do not. The new step polls the exact key the test reads, so "ready" means the same thing to CI as it does to the test, and it fails loudly with container logs instead of letting the test fail on its behalf: ```yaml - name: Wait for Apollo to serve configuration run: | for i in $(seq 1 60); do if curl -sf "http://localhost:8080/configs/SampleApp/default/application" | grep -q server.address; then echo "Apollo is serving configuration." exit 0 fi sleep 5 done echo "Apollo did not start serving configuration within 5 minutes." docker compose -f ".github/workflows/apollo/docker-compose.yml" logs --tail=50 exit 1 ``` ## Also: agollo v4.3.1 -> v4.4.0 Unrelated to the flake — the empty value comes from the server, not the client — but `v4.3.1` is from 2023-07-30 and upstream has had `v4.4.0` since 2024-05-05. Builds and vets clean. `v5.0.0` is left alone: it moves to the `agollo/v5` module path, so it needs import changes and belongs in its own PR. closes #4855 ## Also: the nacos healthcheck has no `start_period` Same theme, opposite failure. Apollo had no healthcheck at all and let an unready service through; nacos has one that is too tight and rejects a service that would have become ready. ``` Container nacos Started Container nacos Waiting Container nacos Error dependency failed to start: container nacos is unhealthy ``` The probe allowed `interval 5s * retries 10` = 50 seconds in total, with no `start_period`. Without it, failures during startup count towards `retries`, so a JVM service that needs longer than 50s on a loaded runner is declared unhealthy — and the `initializer` that waits on `service_healthy` never runs, taking the whole compose down before Go is even installed. `start_period` exists precisely for this: probe failures inside it are not counted. Set to 180s. The probe also lacked `-f`, so `curl` exited 0 for any HTTP status as long as the connection succeeded — a 404 or a 500 would have been reported as healthy. This has hit #4852 and #4856, neither of which touches nacos. --------- Co-authored-by: 码到功成 <739476267@qq.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

@hailaz

gf 2.10.3 Created-by: HarmonybrewBot Commit-by: HarmonybrewBot Merged-by: HarmonybrewBot Description: Created by `brew bump` --- Created with `brew bump-formula-pr`.<details> <summary>release notes</summary> <pre>## What's Changed * fix: update gf cli to v2.10.2 by @github-actions[bot] in gogf/gf#4773 * feat(cmd/gf): gen dao suppport for dm database by @hailaz in gogf/gf#4776 * fix(ghttp): resolve the issue where the GetCertificate callback in TLS listener gets overwritten by @cashtang in gogf/gf#4789 * fix(net/ghttp): report invalid json body errors by @puneetdixit200 in gogf/gf#4775 * fix(gclient): add timeout configuration methods and improve client initialization by @hailaz in gogf/gf#4779 * fix(cmd/gf): missing pointer in generated service file (#4741) by @UncleChair in gogf/gf#4742 * fix(util/gvalid): date validation allows invalid formats like Y-MMMM and invalid dates like 2026-13-33 by @cococolanosugar in gogf/gf#4740 * fix(container/gmap): added WithError methods for KVMap and ListKVMap by @LanceAdd in gogf/gf#4714 * feat(contrib/drivers/mssql): convert uniqueidentifier to uuid.UUID by @NickWilde18 in gogf/gf#4756 * fix(os/glog): JsonHandler outputs flat JSON suitable for ES ingestion (#4790) by @yangyanqing in gogf/gf#4791 * feat(database/gdb): Added OmitZero function to filter zero value parameters by @LanceAdd in gogf/gf#4713 * Revert "feat(database/gdb): Added OmitZero function to filter zero value parameters" by @hailaz in gogf/gf#4818 * fix(net/ghttp): normalize HTTP method to uppercase in getHandlersWithCache to prevent route cache pollution (#4765) by @waterWang in gogf/gf#4812 * fix(os/gcfg): normalize config pattern in GetWithEnv by @webguosai in gogf/gf#4748 * fix(encoding/ghash): PJW64 should use 64-bit width by @guoxing-li in gogf/gf#4728 * fix(net/goai): add style=deepObject and explode=true for query params with object schema (#4784) by @waterWang in gogf/gf#4813 * feat(os/grpool): let can set limit/cap dynamically by @joy999 in gogf/gf#4794 * test(contrib/drivers/pgsql): expand test coverage to MySQL/MariaDB baseline + 3 framework fixes by @lingcoder in gogf/gf#4763 * fix(net/ghttp): use a case-preserving key for the router searching cache by @lingcoder in gogf/gf#4833 * chore(openspec): remove openspec docs from repo by @gqcn in gogf/gf#4838 * fix(util/gconv): support array-backed unmarshalling values by @ErzerLP in gogf/gf#4810 * fix(net/ghttp): windows web admin restart only shutdowns but doesn't restart by @lonevle in gogf/gf#4809 * feat(container/gtree): Add the function of popping left and right nodes of red-black tree by @LanceAdd in gogf/gf#4793 * feat:To support the loong64 architecture upgrade, we have upgraded go-sqlite and modernc.org/libc. by @lijunting2020 in gogf/gf#4847 * fix(util/gconv): unwrap reflect.Value and support pointer element slices by @hailaz in gogf/gf#4848 * fix(net/goai): nil pointer dereference when an array with enum has an interface element (#4746) by @lingcoder in gogf/gf#4844 * ci: update the CI/CD dependency versions and scripts by @lingcoder in gogf/gf#4852 * fix(os/grpool): stop Test_Limit4 asserting a sum that has a race window by @lingcoder in gogf/gf#4854 * fix(contrib/nacos): move to the upstream commit that fixes the RpcClient data race by @lingcoder in gogf/gf#4856 * ci: make the Apollo and nacos containers actually ready before tests run by @lingcoder in gogf/gf#4857 * fix(database/gdb): stop reporting an aborted row iteration as success by @jiang-bx in gogf/gf#4858 * ci: wait for the Apollo database before starting Apollo by @lingcoder in gogf/gf#4860 * test(contrib/drivers): expand gaussdb coverage to the MySQL/PgSQL baseline + 8 related bug fixes by @lingcoder in gogf/gf#4840 * fix(os/grpool): read the array before the queue in Test_Limit4 assertions by @lingcoder in gogf/gf#4862 * fix: v2.10.3 by @hailaz in gogf/gf#4866 ## New Contributors * @cashtang made their first contribution in gogf/gf#4789 * @puneetdixit200 made their first contribution in gogf/gf#4775 * @NickWilde18 made their first contribution in gogf/gf#4756 * @waterWang made their first contribution in gogf/gf#4812 * @webguosai made their first contribution in gogf/gf#4748 * @guoxing-li made their first contribution in gogf/gf#4728 * @ErzerLP made their first contribution in gogf/gf#4810 * @lonevle made their first contribution in gogf/gf#4809 * @lijunting2020 made their first contribution in gogf/gf#4847 * @jiang-bx made their first contribution in gogf/gf#4858 **Full Changelog**: https://github.com/gogf/gf/compare/v2.10.2...v2.10.3</pre> <p>View the full release notes at <a href="https://github.com/gogf/gf/releases/tag/v2.10.3">https://github.com/gogf/gf/releases/tag/v2.10.3</a>.</p> </details> <hr> See merge request: Harmonybrew/homebrew-core!17794

…ent data race (gogf#4856) ## Summary CI fails at random in `contrib/registry/nacos` with a data race that lives entirely inside the SDK: ``` WARNING: DATA RACE Read at 0x00c00037e848 by goroutine 22: (*RpcClient).notifyConnectionEvent() rpc_client.go:487 Previous write at 0x00c00037e848 by goroutine 23: (*RpcClient).reconnect() rpc_client.go:449 ``` No `gf` frame takes part in the racing accesses — this repository only appears at the call entry, `nacos.go:93` calling `clients.NewNamingClient()`. Reported as gogf#4846. ## Why pin a commit In `v2.3.5` the field is plain: ```go currentConnection IConnection // v2.3.5 ``` Upstream made it atomic, which is exactly the right fix: ```go currentConnection atomic.Value // master, stores IConnection ``` That fix is not in any release. `v2.3.5` was pu 8000 blished 2025-10-26 and master is 11 commits ahead of it, so pinning the commit is the only way to pick it up right now. The `go.mod` entries resolve to `v2.3.6-0.20260713020125-4ba94455701e`, and should be moved back to a tag once upstream publishes one. `contrib/config/nacos` was still on `v2.3.3`, two releases behind `contrib/registry/nacos`. Both modules now reference the same version, so the two nacos modules cannot drift apart again. ## Verification - Both modules build and `go vet` clean. - `TestRegistry` and `TestWatch` fail identically before and after the change on a machine with no nacos server running, so the upgrade introduces no regression. Verified against a clean `master` worktree. - The race itself needs a reconnect to trigger, so CI is the real test. closes gogf#4846 Co-authored-by: 码到功成 <739476267@qq.com>

…run (gogf#4857) ## The flake `TestApollo` fails at random on pull requests that touch neither Apollo nor `contrib/` — recently on gogf#4844, gogf#4851 and gogf#4854. ``` [ASSERT] EXPECT == :8000 --- FAIL: TestApollo (10.01s) ``` What passes right before it is the interesting part: ```go adapter, err := apollo.New(ctx, apollo.Config{...}) t.AssertNil(err) // passes t.Assert(config.Available(ctx), true) // passes v, err := config.Get(ctx, `server.address`) t.AssertNil(err) // passes t.Assert(v.String(), ":8000") // fails, the value is empty ``` The client connects and the server answers; the key simply is not there yet. `loads/apollo-quick-start` boots MySQL, imports its seed data, then starts the config service — a `running` container does not mean the configuration is queryable. The adapter cannot report this either: ```go return c.value.Val().(*gjson.Json).Get(pattern).Val(), nil ``` A missing key yields `nil` from `gjson`, and there is no error to return, so an empty value and a genuinely absent key look identical to the caller. ## The fix Nothing waited for the data. The compose file declares no `healthcheck` (`grep -c healthcheck` returns 0) and the workflow moved straight on after `up -d`. Services declared under `services:` in the workflow get the runner built-in health waiting; compose-started ones do not. The new step polls the exact key the test reads, so "ready" means the same thing to CI as it does to the test, and it fails loudly with container logs instead of letting the test fail on its behalf: ```yaml - name: Wait for Apollo to serve configuration run: | for i in $(seq 1 60); do if curl -sf "http://localhost:8080/configs/SampleApp/default/application" | grep -q server.address; then echo "Apollo is serving configuration." exit 0 fi sleep 5 done echo "Apollo did not start serving configuration within 5 minutes." docker compose -f ".github/workflows/apollo/docker-compose.yml" logs --tail=50 exit 1 ``` ## Also: agollo v4.3.1 -> v4.4.0 Unrelated to the flake — the empty value comes from the server, not the client — but `v4.3.1` is from 2023-07-30 and upstream has had `v4.4.0` since 2024-05-05. Builds and vets clean. `v5.0.0` is left alone: it moves to the `agollo/v5` module path, so it needs import changes and belongs in its own PR. closes gogf#4855 ## Also: the nacos healthcheck has no `start_period` Same theme, opposite failure. Apollo had no healthcheck at all and let an unready service through; nacos has one that is too tight and rejects a service that would have become ready. ``` Container nacos Started Container nacos Waiting Container nacos Error dependency failed to start: container nacos is unhealthy ``` The probe allowed `interval 5s * retries 10` = 50 seconds in total, with no `start_period`. Without it, failures during startup count towards `retries`, so a JVM service that needs longer than 50s on a loaded runner is declared unhealthy — and the `initializer` that waits on `service_healthy` never runs, taking the whole compose down before Go is even installed. `start_period` exists precisely for this: probe failures inside it are not counted. Set to 180s. The probe also lacked `-f`, so `curl` exited 0 for any HTTP status as long as the connection succeeded — a 404 or a 500 would have been reported as healthy. This has hit gogf#4852 and gogf#4856, neither of which touches nacos. --------- Co-authored-by: 码到功成 <739476267@qq.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

lingcoder mentioned this pull request Aug 18, 2026

ci: make the Apollo and nacos containers actually ready before tests run #4857

Merged

lingcoder force-pushed the fix/issue-4846-nacos-race branch from 33ba8e7 to 94f50f5 Compare August 18, 2026 12:34

Merge branch 'master' into fix/issue-4846-nacos-race

043a2ed

hailaz approved these changes Aug 19, 2026

View reviewed changes

hailaz merged commit 97fd0f2 into gogf:master Aug 19, 2026
22 checks passed

Navigation Menu

Repository navigation

fix(contrib/nacos): move to the upstream commit that fixes the RpcClient data race - #4856

fix(contrib/nacos): move to the upstream commit that fixes the RpcClient data race#4856
hailaz merged 2 commits into
gogf:masterfrom
lingcoder:fix/issue-4846-nacos-race

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

Summary

Why pin a commit

Verification

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants