Fix gRPC BoardList* methods concurrency issues
#1804
Merged
Conversation
This file contains 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
daemon_fix_and_testsuite
branch
3 times, most recently
from
5f9c668
to
e2ebc8c
Compare
|
I tried this in IDE2, and it does not work as expected. When the indexes update runs, I plug in and detach a board, I see the following panic: After this IDE2 is non-functional: |
|
@kittaakos thanks for the bug report! It should be fixed now, may you try this one? |
|
This version is way more robust than the previous one. Thank you! I still found one issue, however. I do not know the exact steps/timing, but I unplugged two connected boards when the indexes update/core client re-initialization was in progress. Please reference the attached stacktrace: click to see the stacktrace
|
IMHO this is unrelated to the discovery: I see a running And since I'm here I'm going to move the gRPC testsuite on his own PR, so we can review it, it has no sense to keep it here. |
I am positive you know this better, but the panic does not happen from IDE2 |
|
Actually, it happened before the patch: arduino/arduino-ide#681 (comment) |
cmaglie
force-pushed
the
daemon_fix_and_testsuite
branch
from
b777797
to
b438830
Compare
|
I've removed the "testuite" part, this PR should be ready to review now. |
cmaglie
changed the title
BoardList* methods concurrency issues / Add testsuite for gRPCBoardList* methods concurrency issues
umbynos
reviewed
Jul 19, 2022
cmaglie
force-pushed
the
daemon_fix_and_testsuite
branch
from
b438830
to
fc8501c
Compare
cmaglie
added
priority: medium
umbynos
requested changes
Aug 5, 2022
per1234
approved these changes
Aug 8, 2022
There was a problem hiding this comment.
I did some general testing both using the Arduino CLI gRPC interface directly, as well as with the latest build of Arduino IDE 2.x (2.0.0-rc9.1-snapshot-0b33b51). I did not manage to cause any misbehavior, but unfortunately I also was not able to reproduce the issue when using the latest build of Arduino CLI from the master branch.
Codecov Report
@@ Coverage Diff @@
## daemon-fixes #1804 +/- ##
================================================
+ Coverage 36.34% 36.39% +0.04%
================================================
Files 232 232
Lines 19496 19401 -95
================================================
- Hits 7086 7061 -25
+ Misses 11583 11511 -72
- Partials 827 829 +2
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. |
cmaglie
force-pushed
the
daemon_fix_and_testsuite
branch
3 times, most recently
from
7b8d40b
to
d5c39ce
Compare
4 tasks
5 tasks
cmaglie
force-pushed
the
daemon_fix_and_testsuite
branch
from
d5c39ce
to
52499a9
Compare
umbynos
reviewed
Aug 9, 2022
umbynos
approved these changes
Aug 9, 2022
Now the DiscoveryManager is able to start the discoveries and add/remove them in a thread-safe way. Also the watchers may connect and disconnect seamlessly at any time, the incoming events from the discovery are broadcasted correctly to each active watcher. This refactoring dramatically simplifies the DiscoveryManager design.
it will be useful in the next commits.
there is already a cache in the DiscoveryManager there is no need to duplicate it.
otherwise the discovery may send some events before the eventChan is setup (and those events will be lost)
Co-authored-by: Umberto Baldi <34278123+umbynos@users.noreply.github.com>
cmaglie
force-pushed
the
daemon_fix_and_testsuite
branch
from
c3a13cc
to
4c197b8
Compare
3 tasks
3 tasks
6 tasks
cmaglie
added a commit
to cmaglie/arduino-cli
that referenced
this pull request
Mar 24, 2023
Since arduino#1804 has been fixed.
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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)What kind of change does this PR introduce?
Fix the functionality of the discovery manager, making it more robust when
arduino-cliis running as a daemon with many concurrent calls.What is the current behavior?
The behavior is undefined, the most frequent issues are:
BoardListWatchis disconnected and reconnected it will not see new eventsBoardListmultiple times on the same instance may cause issues/crashesWhat is the new behavior?
It should be more resistant to the above cases.
Does this PR introduce a breaking change, and is titled accordingly?
No breaking changes.
Other information:
This PR adds an experimental testsuite to test gRPC function calls. It's a collection of utilities/scripts, I finally found the time to put together.