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.

Sign up for GitHub

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

Jump to bottom

RedisPipeline ignore flush when call dtor from another thread. #736

Merged
merged 2 commits into from
Jan 11, 2023
Merged

RedisPipeline ignore flush when call dtor from another thread. #736

merged 2 commits into from
Jan 11, 2023

Conversation

liuh-80
Copy link
Contributor

@liuh-80 liuh-80 commented Jan 9, 2023

Why I did it

Because RedisPipeline is not thread safe, in c++ if code call exit() from another thread, the dtor of RedisPipeline will be called from another thread and trigger a race condition issue.

How I did it

RedisPipeline ignore flush when call dtor from another thread.

How to verify it

Pass all UT and E2E test cases.

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

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111

Description for the changelog

RedisPipeline ignore flush when call dtor from another thread.

Link to config_db schema for YANG module changes

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

liuh-80
liuh-80 commented Jan 9, 2023
View reviewed changes
common/redispipeline.h
@@ -7,6 +7,10 @@
#include "rediscommand.h"
#include "dbconnector.h"

#include "unistd.h"
#include "sys/syscall.h"
#define gettid() syscall(SYS_gettid)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is no wrapper for gettid() in glibc <= 2.30, so we need use syscall(SYS_gettid)

qiluo-msft
qiluo-msft reviewed Jan 9, 2023
View reviewed changes
common/redispipeline.h
{
// call flush from different thread will trigger race condition issue.
flush();
}
Copy link
Contributor

@qiluo-msft qiluo-msft Jan 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

}

Add a notice logging in else branch. The message could be "RedisPipeline dtor is called from another thread, possibly due to exit()" #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

qiluo-msft
qiluo-msft reviewed Jan 9, 2023
View reviewed changes
common/redispipeline.h
@@ -19,10 +23,16 @@ class RedisPipeline {
, m_remaining(0)
{
m_db = db->newConnector(NEWCONNECTOR_TIMEOUT);
initializeOwnerTid();
}

~RedisPipeline() {
Copy link
Contributor

@qiluo-msft qiluo-msft Jan 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

RedisPipeline

Thanks for fixing the bug. It is from the design that DBConnector could be used only in one thread. Do you find similar issues in other classes in swss-common? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I check all dtor in swss-common only find RedisPipeline class run redis command in dtor.

@liuh-80 liuh-80 marked this pull request as ready for review January 10, 2023 03:16
@liuh-80 liuh-80 merged commit dacbdad into sonic-net:master Jan 11, 2023
@qiluo-msft
Copy link
Contributor

Do we need backport, for example to 202012 branch?

arfeigin pushed a commit to arfeigin/sonic-swss-common that referenced this pull request Jan 23, 2023
@liuh-80 @arfeigin
RedisPipeline ignore flush when call dtor from another thread. (sonic…
dd5f4b5 
…-net#736)

#### Why I did it
Because RedisPipeline is not thread safe, in c++ if code call exit() from another thread, the dtor of RedisPipeline will  be called from another thread and trigger a race condition issue.

#### How I did it
RedisPipeline ignore flush when call dtor from another thread.

#### How to verify it
Pass all UT and E2E test cases.

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

<!--
- Note we only backport fixes to a release branch, *not* features!
- Please also provide a reason for the backporting below.
- e.g.
- [x] 202006
-->

- [ ] 201811
- [ ] 201911
- [ ] 202006
- [ ] 202012
- [ ] 202106
- [ ] 202111

#### Description for the changelog
RedisPipeline ignore flush when call dtor from another thread.

#### Link to config_db schema for YANG module changes
<!--
Provide a link to config_db schema for the table for which YANG model
is defined
Link should point to correct section on https://github.com/Azure/SONiC/wiki/Configuration.
-->

#### A picture of a cute animal (not mandatory but encouraged)
dprital added a commit to dprital/sonic-buildimage that referenced this pull request Jan 30, 2023
@dprital
[submodule] Advance sonic-swss-common pointer
9b285ce 
Update sonic-swss-common submodule pointer to include the following:
* 6b6842a [NotificationProducer] add pipeline support ([sonic-net#708](sonic-net/sonic-swss-common#708))
* 2cb5ea0 Increase the netlink buffer size from 3MB to 16MB. ([sonic-net#739](sonic-net/sonic-swss-common#739))
* dacbdad RedisPipeline ignore flush when call dtor from another thread. ([sonic-net#736](sonic-net/sonic-swss-common#736))

Signed-off-by: dprital <drorp@nvidia.com>
@dprital dprital mentioned this pull request Jan 30, 2023
Merged
8 tasks
liat-grozovik pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Feb 5, 2023
@dprital
[submodule] Advance sonic-swss-common pointer (#13556)
0462874 
Update sonic-swss-common submodule pointer to include the following:
* 6b6842a [NotificationProducer] add pipeline support ([#708](sonic-net/sonic-swss-common#708))
* 2cb5ea0 Increase the netlink buffer size from 3MB to 16MB. ([#739](sonic-net/sonic-swss-common#739))
* dacbdad RedisPipeline ignore flush when call dtor from another thread. ([#736](sonic-net/sonic-swss-common#736))

Signed-off-by: dprital <drorp@nvidia.com>
qiluo-msft pushed a commit that referenced this pull request Feb 10, 2023
@liuh-80 @qiluo-msft
RedisPipeline ignore flush when call dtor from another thread. (#736)
3e34309 
#### Why I did it
Because RedisPipeline is not thread safe, in c++ if code call exit() from another thread, the dtor of RedisPipeline will  be called from another thread and trigger a race condition issue.

#### How I did it
RedisPipeline ignore flush when call dtor from another thread.

#### How to verify it
Pass all UT and E2E test cases.

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

<!--
- Note we only backport fixes to a release branch, *not* features!
- Please also provide a reason for the backporting below.
- e.g.
- [x] 202006
-->

- [ ] 201811
- [ ] 201911
- [ ] 202006
- [ ] 202012
- [ ] 202106
- [ ] 202111

#### Description for the changelog
RedisPipeline ignore flush when call dtor from another thread.

#### Link to config_db schema for YANG module changes
<!--
Provide a link to config_db schema for the table for which YANG model
is defined
Link should point to correct section on https://github.com/Azure/SONiC/wiki/Configuration.
-->

#### A picture of a cute animal (not mandatory but encouraged)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qiluo-msft qiluo-msft qiluo-msft approved these changes

Assignees
No one assigned
Labels
Included in 202012 Branch Request for 202012 Branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@liuh-80 @qiluo-msft