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

Simple Supervisor shutdown array fix #443

Merged
merged 2 commits into from
Apr 27, 2021
Merged

Simple Supervisor shutdown array fix #443

merged 2 commits into from
Apr 27, 2021

Conversation

JackUrb
Copy link
Contributor

@JackUrb JackUrb commented Apr 26, 2021

Right now shutdown isn't being particularly safe iterating over the agents to close. This updates that iteration to pre-populate the list of threads to join.

@JackUrb JackUrb requested a review from mojtaba-komeili April 26, 2021 23:22
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 26, 2021
@JackUrb JackUrb changed the title Simple shutdown array fix Simple Supervisor shutdown array fix Apr 26, 2021
@codecov-commenter
Copy link

codecov-commenter commented Apr 26, 2021

Codecov Report

Merging #443 (529819f) into master (e01d0ff) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #443   +/-   ##
=======================================
  Coverage   66.05%   66.05%           
=======================================
  Files          78       78           
  Lines        7105     7106    +1     
=======================================
+ Hits         4693     4694    +1     
  Misses       2412     2412           
Impacted Files Coverage Δ
mephisto/operations/supervisor.py 78.83% <100.00%> (+0.04%) ⬆️

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 e01d0ff...529819f. Read the comment docs.

@@ -211,7 +211,8 @@ def shutdown(self):
if self.sending_thread is not None:
self.sending_thread.join()
logger.debug(f"Joining agents {self.agents}")
for agent_info in self.agents.values():
agents_to_close = list(self.agents.values())
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a comment explaining the reason for separating it like that here. If I am refactoring this code and I get to to this line, I definitely change it back to what it was before this PR.

Copy link
Contributor

@mojtaba-komeili mojtaba-komeili 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 that comment to avoid future confusions.

Copy link
Contributor

@mojtaba-komeili mojtaba-komeili left a comment

Choose a reason for hiding this comment

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

Great, thank you!

@JackUrb JackUrb merged commit 8adfa23 into master Apr 27, 2021
@JackUrb JackUrb deleted the shutdown-bound-bugfix branch April 27, 2021 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants