Add REDIS async support by martin-belanger · Pull Request #390 · sonic-net/sonic-swss-common

martin-belanger · 2020-10-06T19:03:30Z

This pull request is mainly to add support for REDIS asynchronous APIs (from the hiredis library).

I also fixed a potential memory leak in common/rediscommand.cpp.

…a memory leak would have occured if the methods format() or formatArgv() were to be used more than once on the same object. This change simply ensures that currently allocated memory gets freed before new memory gets allocated.

lguohan · 2020-10-07T05:24:06Z

can you separate the bug fix from this pr? we can review and merge it separately.

qiluo-msft · 2020-10-07T18:51:57Z

common/rediscommand.cpp

+    {
+        redisFreeCommand(temp);
+        temp = nullptr;
+    }


This is a valid bug fix. Could you please add a unit test, so it will fail on old code but pass with your fix? #Closed

Hi @qiluo-msft . I'm not sure how to add a unit test for this. The leak will only happen if someone calls RedisCommand::format() (or RedisCommand::formatArgv()) more than once. The pointer to the leaked memory, temp, is a private member and cannot be tested outside the class RedisCommand itself. I'm running out of ideas on how to write a unit test for this. Any suggestions would be greatly appreciated.

BTW, while looking at the existing unit tests, I found a place where a memory leak would occur. The leak is in file tests/redis_state_ut.cpp, line 738, as follows:

738) RedisCommand keys; 739) keys.format("KEYS %s*", (c.getStateHashPrefix() + tableName).c_str()); 740) RedisReply r(&db, keys, REDIS_REPLY_ARRAY); 741) EXPECT_EQ(r.getContext()->elements, (size_t) 0); 742) // Verify number of objects 743) keys.format("KEYS %s:*", tableName.c_str()); 744) RedisReply r2(&db, keys, REDIS_REPLY_ARRAY); 745) EXPECT_EQ(r2.getContext()->elements, (size_t) numOfKeys);

At line 739 we invoke keys.format() a first time. At line 743 we invoke keys.format() a second time. The second time the function is invoked, the memory that was allocated the first time the function was invoked is not freed and will leak.

Understand the difficulty. Could you separate the bug fix from this pr? The bug fix could be merged sooner.
We need more effort to review the new feature.

In reply to: 501238042 [](ancestors = 501238042)

Ok I will submit a separate pull request

Submitted this pull request that only contains the memory leak fix.
#392

lguohan · 2020-11-26T11:55:13Z

@qiluo-msft, any update on this pr?

qiluo-msft · 2020-11-29T00:48:54Z

common/dbconnector.h

+    int formatted_command(redisCallbackFn * cb_func_p, void * cb_data_p, const char * cmd_p, size_t len);
+
+private:
+    const std::string    db_name_m;


db_name_m [](start = 25, length = 9)

We are using m_cameCase as member variable name convention. For this case, suggest m_dbName #Closed

qiluo-msft · 2020-11-29T00:50:49Z

common/dbconnector.h

+     *
+     * @return User context pointer.
+     */
+    void * get_user_ctx() const { return user_ctx_pm; }


get_user_ctx [](start = 11, length = 12)

Naming convention: cameCaseForFunctionName. So suggest getUserContext() #Closed

qiluo-msft · 2020-11-29T00:51:23Z

common/dbconnector.h

+     * @endcode
+     *
+     */
+    redisAsyncContext * context() const { return ac_pm; }


[](start = 23, length = 1)

Suggest remove extra blank after * #Closed

qiluo-msft · 2020-11-29T00:51:39Z

common/dbconnector.h

+     *
+     * @return The DB name associated with this connector.
+     */
+    const std::string & db_name() const { return db_name_m; }


[](start = 21, length = 1)

Suggest remove extra blank before & #Closed

qiluo-msft · 2020-11-29T00:52:58Z

common/dbconnector.h

+     *
+     * @return The DB name associated with this connector.
+     */
+    const std::string & db_name() const { return db_name_m; }


string [](start = 15, length = 6)

Better return non reference string, so caller does not worry about scope.

Sorry for misleading, I propose to return basic std::string.

In reply to: 532125270 [](ancestors = 532125270)

qiluo-msft · 2020-11-29T00:54:45Z

common/dbconnector.h

+ *          that allows the event loop to dispatch the "events" (i.e. replies
+ *          from the REDIS server) to their corresponding callback functions.
+ */
+class DBConnector_async


DBConnector_async [](start = 6, length = 17)

Could you provide use cases? We also require unit test cases for new classes.

The use cases are described in the Doxygen text above. Basically, this allows designing processes around an event loop such as GLib, libevent, qt, etc. as described in hiredis documentation: https://github.com/redis/hiredis/tree/master/adapters

Thanks I read the Doxygen text.
I am asking about how do you plan to use the new classes in downstream repoes? Do you already have a PR following?
Require unit test cases in this PR. That also serves as use case.

In reply to: 532125398 [](ancestors = 532125398)

This will be used by hamd (pull request: sonic-net/sonic-buildimage#5553).

hamd is the Host Account Management Daemon. It is designed as an asynchronous, non-blocking, event-driven process. It uses the GLib event-driven main-loop framework (ref. https://www.freedesktop.org/software/gstreamer-sdk/data/docs/latest/glib/glib-The-Main-Event-Loop.html).

The hiredis library provides GLib bindings. hamd uses the hiredis asynchronous APIs to make non-blocking, asynchronous, requests to the REDIS server. When responses or events are received from the REDIS server, they get dispatched to callback functions by the GLib main-loop.

The following files show how hamd uses the new async APIs:
https://github.com/martin-belanger/sonic-buildimage/blob/master/src/ham/hamd/hamd_redis.h
https://github.com/martin-belanger/sonic-buildimage/blob/master/src/ham/hamd/hamd_redis.cpp

This shows how hamd's GLib Main-loop is created:
https://github.com/martin-belanger/sonic-buildimage/blob/master/src/ham/hamd/hamd_main.cpp

qiluo-msft · 2020-11-29T00:57:16Z

common/dbconnector.cpp

+    return status;
+}
+
+int DBConnector_async::formatted_command(redisCallbackFn * cb_func_p, void * cb_data_p, const char * cmd_p, size_t len)


formatted_command [](start = 23, length = 17)

command() and formatted_command fit better in the class RedisReply, or its subclass.
Is it a good idea to have a redisCallbackFn member in RedisReply, or its subclass?

The RedisReply class is designed specifically for synchronous calls (i.e. blocking calls). When using the Async library (i.e. non-blocking) we do not wait for a reply. The reply comes back at a later time and is dispatched by the event loop (implemented by GLib, libevent, qt, or others) to a callback function.

Async is a completely different approach and the RedisReply does not apply.

qiluo-msft · 2020-11-29T01:10:19Z

common/dbconnector.h

+    const std::string    db_name_m;
+    const int            db_id_m = -1;
+    std::string          sock_addr_m;
+    redisAsyncContext  * ac_pm = nullptr;


redisAsyncContext [](start = 4, length = 17)

Suggest create a wrapper class: RedisAsyncContext derived from RedisContext.
m_conn should point to ac_pm->c

I cannot derive RedisAsyncContext from RedisContext. RedisAsyncContext is not my definition. It is defined in the hiredis library (a 3rd party library).

Sorry for misleading. I did not mean redisAsyncContext in hiredis repo.
I mean in this repo, create a wrapper class: RedisAsyncContext derived from RedisContext (not redisContext in hiredis repo).

In reply to: 532126556 [](ancestors = 532126556)

I see that a new class named RedisContext was introduced after I initially submitted my pull request. I understand what you are asking.

By the way, since I submitted this pull request (2 months ago) I have been reassigned to a new project. I have very little time for SONiC now, but let me see see what I can do. Maybe one of my colleagues can take this over.

qiluo-msft · 2020-11-29T01:15:45Z

common/table.cpp

+    cmd.formatHSET(key, field_r, value_r);
+    return dbconn_rm.formatted_command(cb_func_p, cb_data_p, cmd.c_str(), cmd.length());
+}
+


Remove extra blank line

qiluo-msft · 2020-11-29T01:17:20Z

common/table.cpp

+    return dbconn_rm.formatted_command(cb_func_p, cb_data_p, cmd.c_str(), cmd.length());
+}
+
+int Table_async::hget(redisCallbackFn * cb_func_p, void * cb_data_p, const std::string & key_r, const std::string & field_r)


hget [](start = 17, length = 4)

Functions in this file fit better in the class DBConnector or its subclass.
It has less relationship with the Table concept. #Closed

I'm just following the same model that was used for class Table. Just like class Table defines Table::hget(), Table::hset(), etc., class Table_async defines Table_async::hget(), Table_async::hset(), etc.

qiluo-msft

As comments. Please also resolve conflicts and add testcase.

qiluo-msft · 2020-11-30T20:45:20Z

common/table.cpp

+
+int Table_async::hget(redisCallbackFn * cb_func_p, void * cb_data_p, const std::string & key_r, const std::string & field_r)
+{
+    std::string  key = getKeyName(key_r);


[](start = 15, length = 2)

Use one blank character

martin-belanger · 2020-12-04T20:34:19Z

I just added a new commit to address the syntax issues (camel-casing, blanks, etc.)

I will look into adding a class RedisAsyncContext derived from RedisContext when I find the time or I will find someone else to do that.

* Add new SSD type support * Fix review comment --------- Co-authored-by: Prince George <[email protected]>

Martin Belanger added 2 commits October 6, 2020 14:49

Add support for REDIS asynchronous APIs

dbe1e2d

martin-belanger mentioned this pull request Oct 6, 2020

Add Host Account Management (HAM) project sonic-net/sonic-buildimage#5553

Open

lguohan requested a review from qiluo-msft October 7, 2020 05:22

qiluo-msft reviewed Oct 7, 2020

View reviewed changes

qiluo-msft reviewed Nov 29, 2020

View reviewed changes

qiluo-msft requested changes Nov 29, 2020

View reviewed changes

qiluo-msft reviewed Nov 30, 2020

View reviewed changes

Martin Belanger added 2 commits December 4, 2020 15:10

Merge latest from upstream Azure/sonic-swss-common

22e1efc

partial changes following code review. More to come.

9c6e040

prgeor added a commit to prgeor/sonic-swss-common that referenced this pull request Feb 27, 2025

Add new SSD type support (sonic-net#390)

ab70e66

* Add new SSD type support * Fix review comment --------- Co-authored-by: Prince George <[email protected]>

Conversation

martin-belanger commented Oct 6, 2020

Uh oh!

lguohan commented Oct 7, 2020

Uh oh!

qiluo-msft Oct 7, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lguohan commented Nov 26, 2020

Uh oh!

qiluo-msft Nov 29, 2020 • 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!

qiluo-msft Nov 29, 2020 • 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!

qiluo-msft Nov 29, 2020 • 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!

qiluo-msft Nov 29, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Nov 29, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martin-belanger commented Dec 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

qiluo-msft Oct 7, 2020 •

edited

Loading

qiluo-msft Nov 29, 2020 •

edited

Loading

qiluo-msft Nov 29, 2020 •

edited

Loading

qiluo-msft Nov 29, 2020 •

edited

Loading

qiluo-msft Nov 29, 2020 •

edited

Loading

qiluo-msft Nov 29, 2020 •

edited

Loading