Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 63 additions & 0 deletions common/dbconnector.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -688,4 +688,67 @@ shared_ptr<string> DBConnector::blpop(const string &list, int timeout)
throw runtime_error("GET failed, memory exception");
}


/*******************************************************************************
/ \ ___ _ _ _ __ ___
/ _ \ / __| | | | '_ \ / __|
/ ___ \\__ \ |_| | | | | (__
/_/ \_\___/\__, |_| |_|\___|
|___/
*******************************************************************************/
DBConnector_async::DBConnector_async(const std::string & db_name,
void * user_ctx_p) :
db_name_m(db_name),
db_id_m(SonicDBConfig::getDbId(db_name_m)),
sock_addr_m(SonicDBConfig::getDbSock(db_name_m)),
user_ctx_pm(user_ctx_p)
{
ac_pm = redisAsyncConnectUnix(sock_addr_m.c_str());

if (ac_pm->err != 0)
{
std::string errmsg("Unable to connect to redis: (" + std::to_string(ac_pm->err) + ')');

if ((ac_pm->errstr != nullptr) && (ac_pm->errstr[0] != '\0'))
errmsg += " " + std::string(ac_pm->errstr);

redisAsyncFree(ac_pm);
ac_pm = nullptr;

throw std::system_error(std::make_error_code(errc::address_not_available), errmsg);
}

ac_pm->data = this;

command(nullptr, nullptr, "SELECT %d", db_id_m);
}

DBConnector_async::~DBConnector_async()
{
if (ac_pm != nullptr)
{
// We can't use redisAsyncFree() here because there may
// be pending messages to be sent or received. redisAsyncDisconnect()
// will ensure that all pending messages are processed before the
// context gets deleted.
redisAsyncDisconnect(ac_pm);
ac_pm = nullptr;
}
}

int DBConnector_async::command(redisCallbackFn * cb_func_p, void * cb_data_p, const char * format_p, ...)
{
va_list ap;
int status;
va_start(ap,format_p);
status = redisvAsyncCommand(ac_pm, cb_func_p, cb_data_p, format_p, ap);
va_end(ap);
return status;
}

int DBConnector_async::formatted_command(redisCallbackFn * cb_func_p, void * cb_data_p, const char * cmd_p, size_t len)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
return redisAsyncFormattedCommand(ac_pm, cb_func_p, cb_data_p, cmd_p, len);
}

}
144 changes: 144 additions & 0 deletions common/dbconnector.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include <memory>

#include <hiredis/hiredis.h>
#include <hiredis/async.h> // redisAsyncContext
#include "rediscommand.h"
#include "redisreply.h"
#define EMPTY_NAMESPACE std::string()
Expand Down Expand Up @@ -171,5 +172,148 @@ void DBConnector::hmset(const std::string &key, InputIterator start, InputIterat
RedisReply r(this, shmset, REDIS_REPLY_STATUS);
}



/*******************************************************************************
/ \ ___ _ _ _ __ ___
/ _ \ / __| | | | '_ \ / __|
/ ___ \\__ \ |_| | | | | (__
/_/ \_\___/\__, |_| |_|\___|
|___/
*******************************************************************************/

/**
* @brief This uses hiredis asynchronous APIs (hiredis/async.h) to connect
* to the REDIS server.
*
* @details Asyncronous architecture means that the application is designed
* around an event loop that calls callback functions for each event.
* The hiredis library provides a number of adapters for 3rd party
* event loop libraries such as GLib, libevent, qt, etc.
* Ref: <a href="https://github.com/redis/hiredis/tree/master/adapters">hiredis/adapters/[*.h]</a>
*
* No additional thread is created when using the async hiredis APIs.
* The hiredis context is simply "hooked up" to the event loop and
* that allows the event loop to dispatch the "events" (i.e. replies
* from the REDIS server) to their corresponding callback functions.
*/
class DBConnector_async
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
public:
/**
* @brief Asynchronous Connector object to the REDIS server.
*
* @param db_name_p Name of the DB we want to connect to (e.g. CONFIG_DB,
* APPL_DB, ...)
*
* @param user_ctx_p Optional user context (future use).
*/
DBConnector_async(const std::string & db_name_p,
void * user_ctx_p=nullptr);
~DBConnector_async();

/**
* @brief Get the hiredis context
*
* @return Return the hiredis async connection context. This API should be
* used when your application needs to hook up the hiredis context
* to the event loop. For example, if your application uses the
* GLib main loop, you would hook up the hiredis context as in the
* example below:
*
* @code
* // --------------------------------------------------------------------
* // Example showing how to hook up the hiredis context retrived
* // from a DBConnector_async object to a GLib main event loop.
* --------------------------------------------------------------------
* #include <glib.h> // g_main_context_default()
* #include <hiredis/adapters/glib.h> // redis_source_new()
* #include "dbconnector.h" // class DBConnector_async
*
* GMainContext * main_ctx_p = g_main_context_default();
* GMainLoop * loop_p = g_main_loop_new(main_ctx_p, FALSE);
* .
* .
*
* DBConnector_async db("APPL_DB");
* .
* .
*
* // Hook up hiredis context to GLib main loop
* g_source_attach(redis_source_new(db.context()), main_ctx_p);
* .
* .
*
* g_main_loop_run(loop_p);
*
* @endcode
*
*/
redisAsyncContext * context() const { return ac_pm; }
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Nov 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[](start = 23, length = 1)

Suggest remove extra blank after * #Closed


/**
* @brief Return the user context that was provided in the constructor.
*
* @return User context pointer.
*/
void * get_user_ctx() const { return user_ctx_pm; }
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Nov 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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


/**
* @brief Get the DB name (e.g. "CONFIG_DB", "APPL_DB", etc.). This is the
* same name that was provided to the constructor.
*
* @return The DB name associated with this connector.
*/
const std::string & db_name() const { return db_name_m; }
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Nov 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[](start = 21, length = 1)

Suggest remove extra blank before & #Closed

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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


/**
* @brief Get the DB ID (e.g. 0 for "APPL_DB", 4 for "CONFIG_DB", etc.)
*
* @return The DB ID associated with this connector.
*/
int db_id() const { return db_id_m; }

/**
* @brief Get the socket address associated with this connector.
*
* @return The Unix Domain Socket name.
*/
const std::string & sock_addr() const { return sock_addr_m; }

/**
* @brief Invoke a REDIS a command
*
* @param cb_func_p Callback function to be invoked when reply is received
* @param cb_data_p User data passed to the callback function
* @param format A format string similar to printf(), but specific to
* hiredis. For more info refer to the documentation for <a
* href="https://github.com/redis/hiredis/blob/master/hiredis.c#L531">redisFormatCommand()</a>.
*
* @return 0 on success, otherwise the status returned by
* redisvAsyncCommand()
*/
int command(redisCallbackFn * cb_func_p, void * cb_data_p, const char * format, ...);

/**
* @brief Invoke a REDIS a command
*
* @param cb_func_p Callback function to be invoked when reply is received
* @param cb_data_p User data passed to the callback function
* @param cmd A formatted command to be sent to the REDIS server
* @param len The length of %cmd
*
* @return 0 on success, otherwise the status returned by
* redisAsyncFormattedCommand()
*/
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;
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Nov 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

const int db_id_m = -1;
std::string sock_addr_m;
redisAsyncContext * ac_pm = nullptr;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

void * user_ctx_pm = nullptr;
};

}
#endif
12 changes: 12 additions & 0 deletions common/rediscommand.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,12 @@ RedisCommand::~RedisCommand()

void RedisCommand::format(const char *fmt, ...)
{
if (temp != nullptr)
{
redisFreeCommand(temp);
temp = nullptr;
}
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Oct 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I will submit a separate pull request

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


va_list ap;
va_start(ap, fmt);
int len = redisvFormatCommand(&temp, fmt, ap);
Expand All @@ -31,6 +37,12 @@ void RedisCommand::format(const char *fmt, ...)

void RedisCommand::formatArgv(int argc, const char **argv, const size_t *argvlen)
{
if (temp != nullptr)
{
redisFreeCommand(temp);
temp = nullptr;
}

int len = redisFormatCommandArgv(&temp, argc, argv, argvlen);
if (len == -1) {
throw std::bad_alloc();
Expand Down
43 changes: 43 additions & 0 deletions common/table.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -241,3 +241,46 @@ string Table::stripSpecialSym(const string &key)

return key;
}


/*******************************************************************************
/ \ ___ _ _ _ __ ___
/ _ \ / __| | | | '_ \ / __|
/ ___ \\__ \ |_| | | | | (__
/_/ \_\___/\__, |_| |_|\___|
|___/
*******************************************************************************/
Table_async::Table_async(DBConnector_async & dbconn_r, const std::string & table_name_r) :
TableBase(table_name_r, SonicDBConfig::getSeparator(dbconn_r.db_name())),
dbconn_rm(dbconn_r)
{
}

Table_async::~Table_async()
{
}

int Table_async::hdel(redisCallbackFn * cb_func_p, void * cb_data_p, const std::string & key_r, const std::string & field_r)
{
std::string key = getKeyName(key_r);
swss::RedisCommand cmd;
cmd.formatHDEL(key, field_r);
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)
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Nov 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
std::string key = getKeyName(key_r);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[](start = 15, length = 2)

Use one blank character

swss::RedisCommand cmd;
cmd.formatHGET(key, field_r);
return dbconn_rm.formatted_command(cb_func_p, cb_data_p, cmd.c_str(), cmd.length());
}

int Table_async::hset(redisCallbackFn * cb_func_p, void * cb_data_p, const std::string & key_r, const std::string & field_r, const std::string & value_r)
{
std::string key = getKeyName(key_r);
swss::RedisCommand cmd;
cmd.formatHSET(key, field_r, value_r);
return dbconn_rm.formatted_command(cb_func_p, cb_data_p, cmd.c_str(), cmd.length());
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove extra blank line

84 changes: 83 additions & 1 deletion common/table.h
Original file line number Diff line number Diff line change
Expand Up @@ -220,5 +220,87 @@ class TableName_KeySet {
std::string getStateHashPrefix() const { return "_"; }
};

}

/*******************************************************************************
/ \ ___ _ _ _ __ ___
/ _ \ / __| | | | '_ \ / __|
/ ___ \\__ \ |_| | | | | (__
/_/ \_\___/\__, |_| |_|\___|
|___/
*******************************************************************************/
/**
* @brief Use hiredis async APIs provided by a DBConnector_async object to
* perform asynchronous Table operations with the REDIS server.
*
*/
class Table_async : public TableBase {
public:
/**
* @brief Provides asynchronous access to a Table in the REDIS DB.
*
* @param dbconn_r Asynchronous DB connector object
* @param table_name_r Name of the Table. E.g. "CONFIG_DB", "APPL_DB",
* etc.
*/
Table_async(DBConnector_async & dbconn_r, const std::string & table_name_r);
~Table_async();

/**
* @brief Perform a HDEL REDIS DB operation
*
* @param cb_func_p Callback function invoked when the HDEL REPLY is
* received from the REDIS server
* @param cb_data_p User data passed to the callback function
* @param key_r Key to be accessed in the Table
* @param field_r Field to HDEL
*
* @return The value returned by redisAsyncFormattedCommand(). Typically,
* that would be REDIS_OK=0 on success, and REDIS_ERR=-1 otherwise.
*/
int hdel(redisCallbackFn * cb_func_p,
void * cb_data_p,
const std::string & key_r,
const std::string & field_r);

/**
* @brief Perform a HGET REDIS DB operation
*
* @param cb_func_p Callback function invoked when the HGET REPLY is
* received from the REDIS server
* @param cb_data_p User data passed to the callback function
* @param key_r Key to be accessed in the Table
* @param field_r Field to HGET
*
* @return The value returned by redisAsyncFormattedCommand(). Typically,
* that would be REDIS_OK=0 on success, and REDIS_ERR=-1 otherwise.
*/
int hget(redisCallbackFn * cb_func_p,
void * cb_data_p,
const std::string & key_r,
const std::string & field_r);

/**
* @brief Perform a HSET REDIS DB operation
*
* @param cb_func_p Callback function invoked when the HSET REPLY is
* received from the REDIS server
* @param cb_data_p User data passed to the callback function
* @param key_r Key to be accessed in the Table
* @param field_r Field to HSET
* @param value_r Value that the field will be HSET to.
*
* @return The value returned by redisAsyncFormattedCommand(). Typically,
* that would be REDIS_OK=0 on success, and REDIS_ERR=-1 otherwise.
*/
int hset(redisCallbackFn * cb_func_p,
void * cb_data_p,
const std::string & key_r,
const std::string & field_r,
const std::string & value_r);

private:
DBConnector_async & dbconn_rm;
};

} // namespace swss
#endif