Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: modify cmdId assignment time to assgining after registering cmdtable #2692

Merged

Conversation

gukj-spel
Copy link
Contributor

Fix #2686
主要解决以下问题

  • pika 之前的 cmdID 赋值是在 cmd 初始函数里,并且会对 PikaCmdTableManager 对象中的 cmdID 自增。由于 PikaCmdTableManager 的 cmdID 是全局唯一,并且没有并发保护。当存在并发构造 cmd 的时候就会引起 data race。

解决方式

  • 调整cmd 赋值与自增时间为注册完 cmdtable 后,由于pika 注册 cmdtable 为单线程,所以不存在并发写问题。

@github-actions github-actions bot added the ☢️ Bug Something isn't working label Jun 2, 2024
@gukj-spel gukj-spel changed the title fix: modify cmdId assignment time to assgining after registering cmdtable. fix: modify cmdId assignment time to assgining after registering cmdtable Jun 2, 2024
@AlexStocks
Copy link
Contributor

相比于加一个接口,把 cmdId 改为原子变量更好点吧,最小改动原则。这个也不会影响性能。

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically.


Rather than adding an interface, it would be better to change cmdId to an atomic variable, based on the principle of minimal changes. This will not affect performance either.

@cheniujh cheniujh requested a review from lqxhub June 2, 2024 13:45
Copy link
Collaborator

@lqxhub lqxhub left a comment

Choose a reason for hiding this comment

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

在命令复制那里, 对应的ID有没有复制过去

@@ -34,7 +34,7 @@ class PikaCmdTableManager {
std::shared_ptr<Cmd> GetCmd(const std::string& opt);
bool CmdExist(const std::string& cmd) const;
CmdTable* GetCmdTable();
uint32_t GetCmdId();
uint32_t GetMaxCmdId();
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里为什么要改成 GetMaxCmdId 而且这个函数应该是没用了吧

Copy link
Contributor Author

@gukj-spel gukj-spel Jun 4, 2024

Choose a reason for hiding this comment

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

这个函数确实没用了,当时想着之后可能有获取全局 cmdId 的需求就保留下来。改名是因为此时获取cmdId 是全局最大的 cmdId,所以改下名字更加直观。

@gukj-spel
Copy link
Contributor Author

gukj-spel commented Jun 4, 2024

在命令复制那里, 对应的ID有没有复制过去

在 PikaCmdTableManager::NewCommand 中会调用 cmd 的 Clone() 函数进行命令复制。

  Cmd* cmd = GetCmdFromDB(opt, *cmds_);
  if (cmd) {
    return std::shared_ptr<Cmd>(cmd->Clone());
  }
  return nullptr;

这里的 Clone() 是 Cmd 基类定义的虚函数,cmdId 是 Cmd基类的成员变量。 一般 Cmd 派生类会重写 Clone() 函数,函数中调用的是 Cmd 派生类的拷贝构造函数。例如下面 ZaddCmd 实现的 Clone() 函数。

  Cmd* Clone() override { return new ZAddCmd(*this); }
  • 如果 Cmd 派生类走默认拷贝构造的话,默认拷贝构造会自动调用基类拷贝构造函数,cmdId 是会被成功复制。
  • 如果Cmd 派生类自定义了拷贝构造,一定要保证其调用基类的拷贝构造函数,否则 cmdId 就无法被正常复制。

刚看了下 pika 所有的 Cmd 派生类,在自定义拷贝构造的时候都调用了基类的拷贝构造函数,所以都能保证 cmdId 的成功复制。

@lqxhub
Copy link
Collaborator

lqxhub commented Jun 4, 2024

在命令复制那里, 对应的ID有没有复制过去

在 PikaCmdTableManager::NewCommand 中会调用 cmd 的 Clone() 函数进行命令复制。

  Cmd* cmd = GetCmdFromDB(opt, *cmds_);
  if (cmd) {
    return std::shared_ptr<Cmd>(cmd->Clone());
  }
  return nullptr;

这里的 Clone() 是 Cmd 基类定义的虚函数,cmdId 是 Cmd基类的成员变量。 一般 Cmd 派生类会重写 Clone() 函数,函数中调用的是 Cmd 派生类的拷贝构造函数。例如下面 ZaddCmd 实现的 Clone() 函数。

  Cmd* Clone() override { return new ZAddCmd(*this); }
  • 如果 Cmd 派生类走默认拷贝构造的话,默认拷贝构造会自动调用基类拷贝构造函数,cmdId 是会被成功复制。
  • 如果Cmd 派生类自定义了拷贝构造,一定要保证其调用基类的拷贝构造函数,否则 cmdId 就无法被正常复制。在

刚看了下 pika 所有的 Cmd 派生类,在自定义拷贝构造的时候都调用了基类的拷贝构造函数,所以都能保证 cmdId 的成功复制。

OK

@AlexStocks AlexStocks merged commit 1705916 into OpenAtomFoundation:unstable Jun 4, 2024
13 checks passed
wangshao1 pushed a commit that referenced this pull request Jun 4, 2024
…able (#2692)

* change cmdId assignment time to assign after intializing cmdtable

* 修改 PikaCmdTableManager中 getCmdId,删除pika_command.cc中多余的头文件引用
AlexStocks pushed a commit that referenced this pull request Jun 12, 2024
* fix:  modify cmdId assignment time to assgining after registering cmdtable (#2692)

* change cmdId assignment time to assign after intializing cmdtable

* 修改 PikaCmdTableManager中 getCmdId,删除pika_command.cc中多余的头文件引用

* fix acl bug

---------

Co-authored-by: Kaijie Gu <[email protected]>
Co-authored-by: liuyuecai <[email protected]>
chejinge pushed a commit that referenced this pull request Jun 12, 2024
* fix:  modify cmdId assignment time to assgining after registering cmdtable (#2692)

* change cmdId assignment time to assign after intializing cmdtable

* 修改 PikaCmdTableManager中 getCmdId,删除pika_command.cc中多余的头文件引用

* fix acl bug

---------

Co-authored-by: Kaijie Gu <[email protected]>
Co-authored-by: liuyuecai <[email protected]>
cheniujh pushed a commit that referenced this pull request Aug 15, 2024
…able (#2692)

* change cmdId assignment time to assign after intializing cmdtable

* 修改 PikaCmdTableManager中 getCmdId,删除pika_command.cc中多余的头文件引用
cheniujh pushed a commit to cheniujh/pika that referenced this pull request Sep 24, 2024
…able (OpenAtomFoundation#2692)

* change cmdId assignment time to assign after intializing cmdtable

* 修改 PikaCmdTableManager中 getCmdId,删除pika_command.cc中多余的头文件引用
cheniujh pushed a commit to cheniujh/pika that referenced this pull request Sep 24, 2024
* fix:  modify cmdId assignment time to assgining after registering cmdtable (OpenAtomFoundation#2692)

* change cmdId assignment time to assign after intializing cmdtable

* 修改 PikaCmdTableManager中 getCmdId,删除pika_command.cc中多余的头文件引用

* fix acl bug

---------

Co-authored-by: Kaijie Gu <[email protected]>
Co-authored-by: liuyuecai <[email protected]>
cheniujh pushed a commit to cheniujh/pika that referenced this pull request Sep 24, 2024
* fix:  modify cmdId assignment time to assgining after registering cmdtable (OpenAtomFoundation#2692)

* change cmdId assignment time to assign after intializing cmdtable

* 修改 PikaCmdTableManager中 getCmdId,删除pika_command.cc中多余的头文件引用

* fix acl bug

---------

Co-authored-by: Kaijie Gu <[email protected]>
Co-authored-by: liuyuecai <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.5.5 4.0.0 ☢️ Bug Something isn't working
Projects
None yet
4 participants