Skip to content

Fixed a couple of (very rare) race conditions #2112

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 2 commits into from
Mar 16, 2023

Conversation

cmaglie
Copy link
Member

@cmaglie cmaglie commented Mar 15, 2023

Please check if the PR fulfills these requirements

See how to contribute

  • The PR has no duplicates (please search among the Pull Requests
    before creating one)
  • The PR follows
    our contributing guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • UPGRADING.md has been updated with a migration guide (for breaking changes)
  • configuration.schema.json updated if new parameters are added.

What kind of change does this PR introduce?

Fixed a couple of theoretical race conditions (they are detected by the race condition detector, but they are very unlikely to happen in a real-world scenario).

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change, and is titled accordingly?

No

Other information

cmaglie added 2 commits March 15, 2023 11:27
The go-routine was spawned before the process was started. This means
that p.Kill may read p.cmd.Process before it is written in p.Run.
@cmaglie cmaglie self-assigned this Mar 15, 2023
@cmaglie cmaglie added topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project labels Mar 15, 2023
@codecov
Copy link

codecov bot commented Mar 15, 2023

Codecov Report

Patch coverage: 25.00% and project coverage change: +0.11 🎉

Comparison is base (245e84c) 35.05% compared to head (2285176) 35.17%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2112      +/-   ##
==========================================
+ Coverage   35.05%   35.17%   +0.11%     
==========================================
  Files         232      232              
  Lines       20480    20498      +18     
==========================================
+ Hits         7180     7210      +30     
+ Misses      12455    12444      -11     
+ Partials      845      844       -1     
Flag Coverage Δ
unit 35.17% <25.00%> (+0.11%) ⬆️

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

Impacted Files Coverage Δ
executils/process.go 78.46% <25.00%> (-4.08%) ⬇️

... and 4 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@cmaglie cmaglie merged commit 5110f0b into arduino:master Mar 16, 2023
@cmaglie cmaglie deleted the fix_race_conditions branch March 16, 2023 14:10
kittaakos pushed a commit to kittaakos/arduino-cli that referenced this pull request Mar 21, 2023
* Fixed concurrent write to variables `err` and `n`

* Fix concurrent access to p.cmd.Process

The go-routine was spawned before the process was started. This means
that p.Kill may read p.cmd.Process before it is written in p.Run.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants