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

buffer & grpcsync: various cleanups and improvements #6785

Merged
merged 3 commits into from
Nov 15, 2023

Conversation

dfawley
Copy link
Member

@dfawley dfawley commented Nov 13, 2023

  • buffer: follow channel semantics for buffer closure
    • notably: closing a channel still results in existing entries being delivered)
  • grpcsync: simplifications in CallbackSerializer
    • esp. with being able to expect channel semantics, the code gets a good bit simpler

RELEASE NOTES: none

@dfawley dfawley added the Type: Internal Cleanup Refactors, etc label Nov 13, 2023
@dfawley dfawley added this to the 1.60 Release milestone Nov 13, 2023
Copy link

codecov bot commented Nov 13, 2023

Codecov Report

Merging #6785 (bc5ba5a) into master (8645f95) will increase coverage by 0.08%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6785      +/-   ##
==========================================
+ Coverage   83.24%   83.33%   +0.08%     
==========================================
  Files         285      285              
  Lines       30950    30937      -13     
==========================================
+ Hits        25765    25782      +17     
+ Misses       4099     4074      -25     
+ Partials     1086     1081       -5     
Files Coverage Δ
internal/buffer/unbounded.go 100.00% <100.00%> (ø)
internal/grpcsync/callback_serializer.go 100.00% <100.00%> (+6.25%) ⬆️

... and 18 files with indirect coverage changes

@arvindbr8 arvindbr8 modified the milestones: 1.60 Release, 1.61 Release Nov 14, 2023
@arvindbr8 arvindbr8 assigned dfawley and unassigned arvindbr8 Nov 15, 2023
@dfawley dfawley merged commit b98104e into grpc:master Nov 15, 2023
14 checks passed
@dfawley dfawley deleted the cleanups branch November 15, 2023 17:32
if len(b.backlog) > 0 {
select {
case b.c <- b.backlog[0]:
b.backlog[0] = nil
b.backlog = b.backlog[1:]
default:
}
} else if b.closing && !b.closed {
Copy link
Contributor

Choose a reason for hiding this comment

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

For correctness sake and to ensure that an extra call to Load() does not close a closed channel, we should set to b.closed to true here.

Comment on lines 41 to +42
closed bool
closing bool
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to move these fields to be under the mu since they are clearly accessed always only with the mutex.

dfawley added a commit to dfawley/grpc-go that referenced this pull request Dec 7, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants