Skip to content

Fix redis client and C string memory leaks in events client - #774

Open
saravanan-i-CLS wants to merge 1 commit into
sonic-net:masterfrom
saravanan-i-CLS:saravanan-i-telemetry-issues-fixed
Open

saravanan-i-CLS wants to merge 1 commit into
sonic-net:masterfrom
saravanan-i-CLS:saravanan-i-telemetry-issues-fixed

Conversation

@saravanan-i-CLS

Copy link
Copy Markdown

Why I did it

The gNMI events client had two resource leaks in sonic_data_client/events_client.go, both reachable in normal operation:

COUNTERS_DB redis client leak. update_stats() creates a redis.Client for COUNTERS_DB once per events subscription and never closes it. Each subscribe/unsubscribe cycle abandoned a client, leaking its TCP socket and internal background goroutines. Over time (repeated telemetry event subscriptions) this accumulates.

C.CString leak. Set_heartbeat() passed C.CString(s) inline to event_set_global_options() without ever freeing it. C.CString always allocates on the C heap and the caller owns the buffer; the callee takes a const char * and does not free it, so every call leaked the string (invoked on each events subscribe that carries a heartbeat path parameter).

How I did it

  • update_stats(): added defer rclient.Close() immediately after the client is created, so it's released on every return path when the subscription is torn down.
  • Set_heartbeat(): capture the C.CString result in a variable and defer C.free(unsafe.Pointer(cs)) (same pattern already used elsewhere in this file and in mixed_db_client.go).
  • Added TestUpdateStatsClosesRedisClient (sonic_data_client/events_client_test.go), a white-box unit test that patches the COUNTERS_DB lookups to an in-memory miniredis, intercepts (*redis.Client).Close, drives update_stats() through a subscribe→stop cycle, and asserts the client is closed.

How to verify it

cd src/sonic-gnmi
go test ./sonic_data_client/ -run TestUpdateStatsClosesRedisClient -v
The test fails without the defer rclient.Close() fix (closeCount == 0) and passes with it. The C.CString fix is verifiable under the LeakSanitizer harness (make check_memleak); note sonic_data_client is currently excluded from that target in the Makefile due to unrelated libyang leaks in the test environment, so the CString fix is covered by code inspection against the existing free pattern.

Which release branch to backport (provide reason below if selected)

Not Applicable

Tested branch

  • master
  • 202305
  • 202311
  • 202405
  • 202411
  • 202505
  • 202511
  • 202605
  • N/A

Test result

master:

$ cd src/sonic-gnmi
$ go test ./sonic_data_client/ -run TestUpdateStatsClosesRedisClient -v
=== RUN TestUpdateStatsClosesRedisClient
--- PASS: TestUpdateStatsClosesRedisClient (~1.0s)
PASS
ok github.com/sonic-net/sonic-gnmi/sonic_data_client

Description for the changelog

[sonic-gnmi] Fix redis client and C string memory leaks in the gNMI events client

Link to config_db schema for YANG module changes

N/A — no CONFIG_DB tables or YANG models are added or changed. This is a code-only resource-cleanup fix in the events data client.

A picture of a cute animal (not mandatory but encouraged)

update_stats() created a COUNTERS_DB redis client per events subscription
but never closed it, leaking a TCP socket and its background goroutines on
every subscribe/unsubscribe cycle. Add a deferred Close().

Set_heartbeat() passed the result of C.CString() inline to the C call and
never freed it. C.CString always allocates and the callee
(event_set_global_options, a const char *) does not take ownership, so the
buffer leaked on every call. Capture the pointer and defer C.free().

Add TestUpdateStatsClosesRedisClient to verify the redis client is closed
on subscription teardown.

Signed-off-by: Saravanan <saravan@celestica.com>
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

@mssonicbld

Copy link
Copy Markdown
Contributor

/azp run

@saravanan-i-CLS
saravanan-i-CLS marked this pull request as ready for review August 31, 2026 06:16
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

@mssonicbld

Copy link
Copy Markdown
Contributor

Hi, there are workflow run(s) waiting for approval, you may be first-time contributor. I will notify maintainers to help approve once PR is approved. Thanks!

---Powered by SONiC BuildBot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants