Fix memory leak issue in ConfigDBConnector. by liuh-80 · Pull Request #655 · sonic-net/sonic-swss-common

liuh-80 · 2022-07-27T07:09:41Z

Why I did it

Fix memory leak issue in ConfigDBConnector:
[chassis] Too many open files error and unable to connect to redis socket error
sonic-net/sonic-buildimage#10870

The reason of this issue is DBConnector::pubsub() will return a pointer, and following code call this method but never release the returned pointer:

void ConfigDBConnector_Native::db_connect(string db_name, bool wait_for_init, bool retry_on)
{
    m_db_name = db_name;
    m_key_separator = m_table_name_separator = get_db_separator(db_name);
    SonicV2Connector_Native::connect(m_db_name, retry_on);

    if (wait_for_init)
    {
        auto& client = get_redis_client(m_db_name);
        auto pubsub = client.pubsub(); <== this pointer not delete later.

Also change DBConnector::pubsub() to deprecated for none SWIG scenario.

How I did it

Change DBConnector::pubsub() to return a smart pointer.

How to verify it

Pass all test case.

Run following code in python and validate there is no epoll and socket leak:

import gc
from swsscommon import swsscommon
config_db = swsscommon.ConfigDBConnector_Native()
config_db.connect()
config_db.connect()
config_db.connect()

gc.collect()

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

Description for the changelog

Fix epoll and socket resurce leak issue:
[chassis] Too many open files error and unable to connect to redis socket error
sonic-net/sonic-buildimage#10870

Link to config_db schema for YANG module changes

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

common/dbconnector.h

liuh-80 · 2022-07-27T07:11:05Z

common/dbconnector.h

    DBConnector *newConnector(unsigned int timeout) const;

-    PubSub *pubsub();
+    std::shared_ptr<PubSub> pubsub();


Created a draft PR to validate this change will not break existed code:
sonic-net/sonic-buildimage#11540

Build image PR passed all test.

common/dbconnector.h

liuh-80 · 2022-07-28T04:51:57Z

common/configdb.cpp

    if (wait_for_init)
    {
        auto& client = get_redis_client(m_db_name);
-        auto pubsub = client.pubsub();


Code change here because pubsub() mark as deprecated.

liuh-80 · 2022-07-28T09:48:35Z

/azp run

azure-pipelines · 2022-07-28T09:48:46Z

Azure Pipelines successfully started running 1 pipeline(s).

qiluo-msft · 2022-07-28T18:23:22Z

Please check this unit test failure

tests/test_countertable.py .swig/python detected a memory leak of type 'std::string *', no destructor found.

In reply to: 1198490986

liuh-80 · 2022-07-29T05:55:23Z

Please check this unit test failure

tests/test_countertable.py .swig/python detected a memory leak of type 'std::string *', no destructor found.

Fixed, False alert because UT time out. add a new UT to cover the changed code.

* Fix memory leak issue in ConfigDBConnector. (sonic-net#655) #### Why I did it Fix memory leak issue in ConfigDBConnector: [chassis] Too many open files error and unable to connect to redis socket error sonic-net/sonic-buildimage#10870 The reason of this issue is DBConnector::pubsub() will return a pointer, and following code call this method but never release the returned pointer: ``` void ConfigDBConnector_Native::db_connect(string db_name, bool wait_for_init, bool retry_on) { m_db_name = db_name; m_key_separator = m_table_name_separator = get_db_separator(db_name); SonicV2Connector_Native::connect(m_db_name, retry_on); if (wait_for_init) { auto& client = get_redis_client(m_db_name); auto pubsub = client.pubsub(); <== this pointer not delete later. ``` Also change DBConnector::pubsub() to deprecated for none SWIG scenario. #### How I did it Change DBConnector::pubsub() to return a smart pointer. #### How to verify it Pass all test case. Run following code in python and validate there is no epoll and socket leak: ``` import gc from swsscommon import swsscommon config_db = swsscommon.ConfigDBConnector_Native() config_db.connect() config_db.connect() config_db.connect() gc.collect() ``` #### Which release branch to backport (provide reason below if selected)  - [ ] 201811 - [ ] 201911 - [ ] 202006 - [ ] 202012 - [ ] 202106 - [x] 202111 #### Description for the changelog Fix epoll and socket resurce leak issue: [chassis] Too many open files error and unable to connect to redis socket error sonic-net/sonic-buildimage#10870 #### Link to config_db schema for YANG module changes #### A picture of a cute animal (not mandatory but encouraged) Co-authored-by: liuh-80 <azureuser@liuh-dev-vm-02.5fg3zjdzj2xezlx1yazx5oxkzd.hx.internal.cloudapp.net> * Transfer organization from Azure to sonic-net (sonic-net#656) Transfer organization from Azure to sonic-net * Add docker-mux related table names (sonic-net#627) This PR is to add table name definitions for database entries used by docker mux processes. Sign-off: Jing Zhang [email protected] * Add libzmq dependency * Add test logs * Add libzmq3-dev dependency * Add libboost-serialization and uuid-dev dependencies * Add boost and uuid * Add installation before building docker Co-authored-by: Hua Liu <[email protected]> Co-authored-by: liuh-80 <azureuser@liuh-dev-vm-02.5fg3zjdzj2xezlx1yazx5oxkzd.hx.internal.cloudapp.net> Co-authored-by: Liu Shilong <[email protected]> Co-authored-by: Jing Zhang <[email protected]> Co-authored-by: Ubuntu <zain@zb-dev-vm.022x1jpnpm4u1iy2d325acts3c.yx.internal.cloudapp.net>

SuvarnaMeenakshi · 2022-08-03T23:52:24Z

@liuh-80 should change be included in 202205 branch?

qiluo-msft · 2022-09-10T23:22:21Z

@liuh-80 Do we need to backport to older branches like 202012?

yxieca · 2022-09-21T21:46:19Z

@liuh-80 this change cannot be cherry-picked to 202205 branch cleanly, can you raise a separate PR?

abdosi · 2022-10-22T00:06:53Z

@liuh-80 please create PR for 2022205
cc @SuvarnaMeenakshi for viz.

Fix memory leak issue in ConfigDBConnector: [chassis] Too many open files error and unable to connect to redis socket error sonic-net/sonic-buildimage#10870 The reason of this issue is DBConnector::pubsub() will return a pointer, and following code call this method but never release the returned pointer: ``` void ConfigDBConnector_Native::db_connect(string db_name, bool wait_for_init, bool retry_on) { m_db_name = db_name; m_key_separator = m_table_name_separator = get_db_separator(db_name); SonicV2Connector_Native::connect(m_db_name, retry_on); if (wait_for_init) { auto& client = get_redis_client(m_db_name); auto pubsub = client.pubsub(); <== this pointer not delete later. ``` Also change DBConnector::pubsub() to deprecated for none SWIG scenario. Change DBConnector::pubsub() to return a smart pointer. Pass all test case. Run following code in python and validate there is no epoll and socket leak: ``` import gc from swsscommon import swsscommon config_db = swsscommon.ConfigDBConnector_Native() config_db.connect() config_db.connect() config_db.connect() gc.collect() ```  - [ ] 201811 - [ ] 201911 - [ ] 202006 - [ ] 202012 - [ ] 202106 - [x] 202111 Fix epoll and socket resurce leak issue: [chassis] Too many open files error and unable to connect to redis socket error sonic-net/sonic-buildimage#10870 Co-authored-by: liuh-80 <azureuser@liuh-dev-vm-02.5fg3zjdzj2xezlx1yazx5oxkzd.hx.internal.cloudapp.net>

abdosi · 2022-11-11T17:06:46Z

@liuh-80 can we have PR for 2022205. This is very critical for Chassis/multi-asic environment.

cc @rlhui for viz

SuvarnaMeenakshi · 2022-11-11T17:13:36Z

@liuh-80 can we have PR for 2022205. This is very critical for Chassis/multi-asic environment.

cc @rlhui for viz

@abdosi There is a PR for 202205 #706.

Fix memory leak issue in ConfigDBConnector: [chassis] Too many open files error and unable to connect to redis socket error sonic-net/sonic-buildimage#10870 The reason of this issue is DBConnector::pubsub() will return a pointer, and following code call this method but never release the returned pointer: ``` void ConfigDBConnector_Native::db_connect(string db_name, bool wait_for_init, bool retry_on) { m_db_name = db_name; m_key_separator = m_table_name_separator = get_db_separator(db_name); SonicV2Connector_Native::connect(m_db_name, retry_on); if (wait_for_init) { auto& client = get_redis_client(m_db_name); auto pubsub = client.pubsub(); <== this pointer not delete later. ``` Also change DBConnector::pubsub() to deprecated for none SWIG scenario. Change DBConnector::pubsub() to return a smart pointer. Pass all test case. Run following code in python and validate there is no epoll and socket leak: ``` import gc from swsscommon import swsscommon config_db = swsscommon.ConfigDBConnector_Native() config_db.connect() config_db.connect() config_db.connect() gc.collect() ```  - [ ] 201811 - [ ] 201911 - [ ] 202006 - [ ] 202012 - [ ] 202106 - [x] 202111 Fix epoll and socket resurce leak issue: [chassis] Too many open files error and unable to connect to redis socket error sonic-net/sonic-buildimage#10870 Co-authored-by: liuh-80 <azureuser@liuh-dev-vm-02.5fg3zjdzj2xezlx1yazx5oxkzd.hx.internal.cloudapp.net> Co-authored-by: liuh-80 <azureuser@liuh-dev-vm-02.5fg3zjdzj2xezlx1yazx5oxkzd.hx.internal.cloudapp.net>

Fix memory leak issue.

7759b78

liuh-80 commented Jul 27, 2022

View reviewed changes

common/dbconnector.h Show resolved Hide resolved

liuh-80 commented Jul 27, 2022

View reviewed changes

liuh-80 added 2 commits July 27, 2022 08:29

Fix return object issue

14ca06d

Fix name space issue

debe508

liuh-80 marked this pull request as ready for review July 28, 2022 00:12

liuh-80 requested a review from qiluo-msft July 28, 2022 00:12

qiluo-msft reviewed Jul 28, 2022

View reviewed changes

common/dbconnector.h Outdated Show resolved Hide resolved

Improve code by PR comments

f211ecb

liuh-80 commented Jul 28, 2022

View reviewed changes

liuh-80 mentioned this pull request Jul 28, 2022

[sonic-swss-common] Update sonic-swss-common submodule sonic-net/sonic-buildimage#11524

Merged

5 tasks

qiluo-msft previously approved these changes Jul 28, 2022

View reviewed changes

Improve code coverage

85da7fd

liuh-80 dismissed qiluo-msft’s stale review via 85da7fd July 28, 2022 08:40

Add new UT to improve code coverage

2600d2c

qiluo-msft approved these changes Jul 29, 2022

View reviewed changes

liuh-80 merged commit 2247dbe into sonic-net:master Aug 1, 2022

SuvarnaMeenakshi mentioned this pull request Aug 17, 2022

[chassis] Too many open files error and unable to connect to redis socket error sonic-net/sonic-buildimage#10870

Closed

qiluo-msft added Request for 202111 Branch Request for 202205 Branch labels Sep 10, 2022

yxieca added the Cherry Pick Conflict_202205 label Sep 21, 2022

abdosi added the Chassis label Oct 22, 2022

yxieca added the Included in 202205 Branch label Dec 12, 2022

Conversation

liuh-80 commented Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why I did it

How I did it

How to verify it

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

Description for the changelog

Link to config_db schema for YANG module changes

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

Uh oh!

Uh oh!

liuh-80 Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liuh-80 Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

liuh-80 Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

liuh-80 commented Jul 28, 2022

Uh oh!

azure-pipelines bot commented Jul 28, 2022

Uh oh!

qiluo-msft commented Jul 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liuh-80 commented Jul 29, 2022

Uh oh!

SuvarnaMeenakshi commented Aug 3, 2022

Uh oh!

qiluo-msft commented Sep 10, 2022

Uh oh!

yxieca commented Sep 21, 2022

Uh oh!

abdosi commented Oct 22, 2022

Uh oh!

abdosi commented Nov 11, 2022

Uh oh!

SuvarnaMeenakshi commented Nov 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

liuh-80 commented Jul 27, 2022 •

edited

Loading

liuh-80 Jul 27, 2022 •

edited

Loading

qiluo-msft commented Jul 28, 2022 •

edited

Loading