[breaking] Fix gRPC streaming synchronization on close #1787
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
added
criticality: highest
per1234
added
topic: code
cmaglie
force-pushed
the
fix_grpc_streaming_sync_on_close
branch
from
9a0f393 to
d9b973a
Compare
|
I've also found that |
Well, actually I found an implementation of buffered pipes under MIT license, that seems to fix the issue. I've updated the PR, now it should be production-ready. |
cmaglie
force-pushed
the
fix_grpc_streaming_sync_on_close
branch
from
94ad507 to
08f22db
Compare
|
linked to #1073, this change resolves the issue of missing compile output in the IDE. |
per1234
requested changes
Jun 30, 2022
Co-authored-by: per1234 <[email protected]>
per1234
approved these changes
Jul 1, 2022
There was a problem hiding this comment.
This fixes arduino/arduino-ide#1073 for me.
Thanks Cristian!
umbynos
approved these changes
Jul 1, 2022
3 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?
Adds some synchronization to properly handle streaming close.
What is the current behavior?
Sometimes the last part of the streaming call is lost (it could be the last piece of the compile output for example).
Moreover, the gRPC thread is leaked, even if the amount of resources leakied is negligible, this may add up if the daemon is left running for a long time with a huge number of compiles.
This issue has been there for a long time, the gRPC rate-limiting implemented here #1759 just made it more visible.
What is the new behavior?
The streaming synchronization is now properly handled.
Does this PR introduce a breaking change, and is titled accordingly?
The function that we use internally
FeedStreamTohas been changed.Anyway since this is a public-facing API the change has been documented.