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 potential race condition #248

Merged
3 commits merged into from
Dec 6, 2022

Conversation

ryanolson
Copy link
Contributor

Prior to this PR, there was a possible race condition that could be hit by two execution paths where,

  • path 1 calls Server::drop_stream which is a yielding call
  • path 2 call Server:m_connections->drop_all_streams();

Server::drop_stream locks the server's state, but Server::m_connections is not protected by that mutex.

Relates to #247

…ream and another fiber calling a public member of server::ConnectionsManager::drop_all_streams
@ryanolson ryanolson added bug Something isn't working non-breaking Non-breaking change labels Dec 5, 2022
@ryanolson ryanolson requested a review from a team as a code owner December 5, 2022 22:09
@ryanolson ryanolson self-assigned this Dec 5, 2022
@ryanolson
Copy link
Contributor Author

Note, this will pass CI, because CI is still skipping test_mrc_private.x.

@ryanolson ryanolson mentioned this pull request Dec 5, 2022
2 tasks
@ryanolson
Copy link
Contributor Author

ryanolson commented Dec 5, 2022

Note: a boost::fibers::recursive_mutex is not an option due to the condition variable

@codecov
Copy link

codecov bot commented Dec 5, 2022

Codecov Report

Merging #248 (39dc6fe) into branch-23.01 (6f7b4ad) will decrease coverage by 0.03%.
The diff coverage is 0.00%.

Additional details and impacted files

Impacted file tree graph

@@               Coverage Diff                @@
##           branch-23.01     #248      +/-   ##
================================================
- Coverage         46.00%   45.96%   -0.04%     
================================================
  Files               335      335              
  Lines              9278     9288      +10     
  Branches            774      775       +1     
================================================
+ Hits               4268     4269       +1     
- Misses             5010     5019       +9     
Flag Coverage Δ
cpp 37.58% <0.00%> (-0.05%) ⬇️
py 39.21% <0.00%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cpp/mrc/src/internal/control_plane/server.cpp 0.00% <0.00%> (ø)
cpp/mrc/src/internal/control_plane/server.hpp 0.00% <ø> (ø)
...ternal/control_plane/server/connection_manager.cpp 0.00% <0.00%> (ø)
cpp/mrc/src/internal/resources/manager.cpp 54.02% <ø> (ø)
cpp/mrc/src/internal/system/fiber_task_queue.cpp 65.00% <0.00%> (+2.50%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f7b4ad...39dc6fe. Read the comment docs.

@ryanolson
Copy link
Contributor Author

@gpucibot merge

@ghost ghost merged commit 0db47ad into nv-morpheus:branch-23.01 Dec 6, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working non-breaking Non-breaking change
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants