[breaking] daemon: Fix concurrency and streamline access to PackageManager #1828
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cmaglie
force-pushed
the
allow_core_search_and_init
branch
from
e4ed93a to
de6ae8c
Compare
Codecov Report
@@ Coverage Diff @@
## daemon-fixes #1828 +/- ##
================================================
- Coverage 36.47% 36.33% -0.14%
================================================
Files 232 231 -1
Lines 19401 19550 +149
================================================
+ Hits 7076 7103 +27
- Misses 11500 11617 +117
- Partials 825 830 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
4 tasks
3 tasks
cmaglie
added
priority: high
kittaakos
pushed a commit
to arduino/arduino-ide
that referenced
this pull request
Aug 10, 2022
Ref: arduino/arduino-cli#1828 Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
kittaakos
pushed a commit
to arduino/arduino-ide
that referenced
this pull request
Aug 10, 2022
Ref: arduino/arduino-cli#1828 Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
kittaakos
pushed a commit
to arduino/arduino-ide
that referenced
this pull request
Aug 10, 2022
Ref: arduino/arduino-cli#1828 Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
4 tasks
It has been splitted and merged into HardwareLoader and TargetBoardResolver that are more appropriate.
cmaglie
force-pushed
the
allow_core_search_and_init
branch
from
de6ae8c to
6e00737
Compare
|
This PR now targets arduino:daemon-fixes branch, the docs will be updated later or in another PR. |
umbynos
suggested changes
Aug 11, 2022
| } | ||
|
|
||
| pm := pmb.Build() | ||
| pme, _ /* never release... */ := pm.NewExplorer() |
There was a problem hiding this comment.
this is really ugly, maybe bringing this inside the arduino builder should solve the problem
There was a problem hiding this comment.
Unfortunately it's more complicated than that, we have to keep it for now.
Let's open a dedicated issue to keep track of it and fix it in the future.
per1234
added
topic: code
per1234
requested changes
Aug 11, 2022
They have been moved into a Builder object that has the ability to build a new PackageManager. This allows to clearly separate subrotuines that actually change the status of the PackageManager from subroutines that just need to query it.
The Explorer object can be see as a read-only "view" to the underlying PackageManager: we may ask the PackageManager to create an Explorer on itself. The returned explorer will held a read-lock on the PackageManager until it's disposed. This architecture should prevent unwanted changes on the PackageManager while it's being used, and viceversa, when the PackageManager is updated it should be guaranteed that no Explorers are reading it.
cmaglie
force-pushed
the
allow_core_search_and_init
branch
3 times, most recently
from
7ed5863 to
721e407
Compare
… calling commands.Init Otherwise, since Init will try to take a write-lock, it will block indefinitely.
This container will handle all the atomic access to the instances map.
It has also been deprecated in favor of GetPackageManagerExplorer.
Previuosly the methods PackageManager.NewBuilder and PackageManager.NewExplorer were available also on Builder and Explorer. Now Builder and Explorer does not inherith these methods anymore, avoiding trivial errors like the one fixed in this commit in the builder_utils package.
cmaglie
force-pushed
the
allow_core_search_and_init
branch
2 times, most recently
from
249193a to
23694ca
Compare
cmaglie
force-pushed
the
allow_core_search_and_init
branch
from
23694ca to
04e70c5
Compare
per1234
requested changes
Aug 24, 2022
cmaglie
force-pushed
the
allow_core_search_and_init
branch
from
45844a8 to
afbed02
Compare
Co-authored-by: per1234 <accounts@perglass.com>
cmaglie
force-pushed
the
allow_core_search_and_init
branch
from
009dc94 to
df28c7c
Compare
per1234
approved these changes
Aug 26, 2022
There was a problem hiding this comment.
I verified that the recent round of changes fixed the known defect in the IDE that occurred when using the previous version: arduino/arduino-ide#1330
I have also done a lot of general testing of the Arduino IDE 2.x integration without discovering any problems.
Thanks for your excellent work both on the code and the documentation Cristian!
3 tasks
4 tasks
3 tasks
3 tasks
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Please check if the PR fulfills these requirements
before creating one)
our contributing guidelines
UPGRADING.mdhas been updated with a migration guide (for breaking changes)Makes access to PackageManger thread-safe and transactional allowing multiple thread concurrent access.
This is not a problem when the arduino-cli is run from the command line (since there is no concurrency in this case), but it can be problematic in daemon mode, when a thread may read the package manager status while another one is writing on it (for example a thread may do a
BoardSearchwhile another one is doing aPlatformsInstallor anInit).The CLI daemon may crash in some specific circumstances.
The CLI daemon should handle gracefully any combination of gRPC requests.
Yes, docs are
not yetupdated.This PR is built on top of Fix gRPC
BoardList*methods concurrency issues #1804 and must be merged together with it.