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

daemon: fix data race when accessing requestedRestart #13987

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zyga
Copy link
Collaborator

@zyga zyga commented May 21, 2024

This was flagged by TIOBE as CID:NA-0e2ca076d1d5a16a07303509981ae85d

This was flagged by TIOBE as CID:NA-0e2ca076d1d5a16a07303509981ae85d

Signed-off-by: Zygmunt Krynicki <[email protected]>
Copy link
Collaborator

@andrewphelpsj andrewphelpsj left a comment

Choose a reason for hiding this comment

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

Thanks @zyga and TIOBE!

@github-actions github-actions bot added the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label May 21, 2024
@zyga zyga removed the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label May 21, 2024
Copy link
Contributor

@MiguelPires MiguelPires left a comment

Choose a reason for hiding this comment

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

LGTM but there's another access to d.requestedRestart towards the end of the method that could use the same fix (line 587)

d.mu.Unlock()

// before not accepting any new client connections we need to write the
// maintenance.json file for potential clients to see after the daemon stops
// responding so they can read it correctly and handle the maintenance
if err := d.updateMaintenanceFile(d.requestedRestart); err != nil {
if err := d.updateMaintenanceFile(requestedRestart); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

as @MiguelPires pointed out there's one other place accessing d.requestRestart directly, and then one more accessing/modifying d.restartSocket

@zyga
Copy link
Collaborator Author

zyga commented Jun 5, 2024

This needs some more work. I have alot bigger version but I didn't finish it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants