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 state machine handler race conditions introduced by strand::wrap #257

Merged
merged 3 commits into from
Aug 6, 2024

Conversation

lolyu
Copy link
Contributor

@lolyu lolyu commented Jun 24, 2024

Description of PR

Summary:
Fixes # (issue)

Type of change

  • Bug fix
  • New feature
  • Doc/Design
  • Unit test

Approach

What is the motivation for this PR?

As the subject.

Work item tracking
  • Microsoft ADO (number only): 28585413

How did you do it?

Post the handler directly to handler instead of calling it with strand::wrap

How did you verify/test it?

UT

Any platform specific information?

Documentation

@lolyu lolyu force-pushed the fix_race_with_strand_wrap branch 2 times, most recently from e37ec72 to abc122f Compare July 1, 2024 08:39
@lolyu lolyu marked this pull request as ready for review July 1, 2024 08:52
@lolyu lolyu requested a review from yxieca July 1, 2024 08:53
@lolyu
Copy link
Contributor Author

lolyu commented Jul 1, 2024

cannot repro the UT failure on local device, will investigate soon.

@lolyu lolyu changed the title Fix state machine handler race conditions introduced by strand::wrap [WIP] Fix state machine handler race conditions introduced by strand::wrap Jul 1, 2024
@lolyu lolyu removed the request for review from yxieca July 1, 2024 09:57
@yxieca
Copy link
Contributor

yxieca commented Jul 1, 2024

@lolyu please look into the checker failures.

@lolyu
Copy link
Contributor Author

lolyu commented Jul 2, 2024

@lolyu please look into the checker failures.

Sure, it is a pretty tricky regression due to the change, will fix soon.

Signed-off-by: Longxiang Lyu <lolv@microsoft.com>
@lolyu lolyu force-pushed the fix_race_with_strand_wrap branch 2 times, most recently from 28c057a to 34ac1e0 Compare July 3, 2024 07:41
Signed-off-by: Longxiang Lyu <lolv@microsoft.com>
@lolyu lolyu force-pushed the fix_race_with_strand_wrap branch from a74a321 to b16de0f Compare July 3, 2024 08:15
@lolyu lolyu changed the title [WIP] Fix state machine handler race conditions introduced by strand::wrap Fix state machine handler race conditions introduced by strand::wrap Jul 3, 2024
@lolyu lolyu requested a review from yxieca July 3, 2024 08:16
Copy link
Contributor

@zjswhhh zjswhhh left a comment

Choose a reason for hiding this comment

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

When will the

test/FakeMuxPort.cpp Show resolved Hide resolved
src/MuxPort.cpp Show resolved Hide resolved
Copy link
Contributor

@zjswhhh zjswhhh left a comment

Choose a reason for hiding this comment

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

Please add PR description.

@lolyu
Copy link
Contributor Author

lolyu commented Jul 4, 2024

Please add PR description.

the PR description is there

Copy link
Contributor

@zjswhhh zjswhhh left a comment

Choose a reason for hiding this comment

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

lgtm

@lolyu lolyu merged commit 22f55a7 into sonic-net:master Aug 6, 2024
9 checks passed
mssonicbld pushed a commit to mssonicbld/sonic-linkmgrd that referenced this pull request Aug 23, 2024
sonic-net#257)

Approach
What is the motivation for this PR?
As the subject.

Work item tracking
Microsoft ADO (number only): 28585413
How did you do it?
Post the handler directly to handler instead of calling it with strand::wrap

How did you verify/test it?
UT

Any platform specific information?
Documentation
@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202405: #272

mssonicbld pushed a commit that referenced this pull request Aug 24, 2024
#257)

Approach
What is the motivation for this PR?
As the subject.

Work item tracking
Microsoft ADO (number only): 28585413
How did you do it?
Post the handler directly to handler instead of calling it with strand::wrap

How did you verify/test it?
UT

Any platform specific information?
Documentation
mssonicbld pushed a commit to mssonicbld/sonic-linkmgrd that referenced this pull request Aug 28, 2024
sonic-net#257)

Approach
What is the motivation for this PR?
As the subject.

Work item tracking
Microsoft ADO (number only): 28585413
How did you do it?
Post the handler directly to handler instead of calling it with strand::wrap

How did you verify/test it?
UT

Any platform specific information?
Documentation
@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202311: #274

mssonicbld pushed a commit that referenced this pull request Aug 28, 2024
#257)

Approach
What is the motivation for this PR?
As the subject.

Work item tracking
Microsoft ADO (number only): 28585413
How did you do it?
Post the handler directly to handler instead of calling it with strand::wrap

How did you verify/test it?
UT

Any platform specific information?
Documentation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants