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 sideband race condition by passing std::string instead of char* to thread proc #1131

Merged
merged 1 commit into from
Nov 27, 2024

Conversation

asumit
Copy link
Contributor

@asumit asumit commented Nov 27, 2024

What does this Pull Request accomplish?

Fixes a race condition in the sideband streaming code path.
GetOwnerSidebandDataToken inside the read/write loop of the server waits on being passed in the right "usage_id".
BeginSidebandStreaming calls the read/write loop in a separate thread at the server and passes in the usage_id which was stored in a stack variable of char[].

std::thread takes in the Function RunSidebandReadWriteLoop that has a string parameter for the usage_id.
When a char[] object is passed to the method, it does try to do a deep copy of the params for thread execution.
In between there is some time that the calling thread stack can unwind removing the memory for the usage_id. If that happens and is happening always unless we are debugging and we know where to delay.

Instead we would convert that char[] to a string object that will get passed to the RunSidebandReadWriteLoop . The std::string copy constructor ensures that the data is already deep copied that is passed for the thread object creation which happens on the current thread and not the new thread. So it ensures that when the std::thread does its own deep copy of the thread params, it has a valid copy
ref : https://stackoverflow.com/questions/65518555/stdthread-arguments-are-copied-on-the-new-thread
ref : https://en.cppreference.com/w/cpp/thread/thread/thread

Why should this Pull Request be merged?

fixes a race condition for sideband streaming.

What testing has been done?

Tested the server and client app locally and it runs successfully.

@doshirohan doshirohan merged commit 06ed455 into main Nov 27, 2024
9 of 10 checks passed
@doshirohan doshirohan deleted the users/sagrahar/streaming-race-condition branch November 27, 2024 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants