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

internal: use OS defaults for TCP keepalive params only on unix #6841

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

easwars
Copy link
Contributor

@easwars easwars commented Dec 8, 2023

In #6834, we made a change to ensure that OS defaults for TCP keepalive params are used. But, that change failed to compile on windows. This fix addresses it.

RELEASE NOTES: none

@easwars easwars requested a review from dfawley December 8, 2023 21:09
@easwars easwars added this to the 1.60 Release milestone Dec 8, 2023
Copy link

codecov bot commented Dec 8, 2023

Codecov Report

Merging #6841 (6c63504) into master (477bd62) will decrease coverage by 0.11%.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6841      +/-   ##
==========================================
- Coverage   83.65%   83.54%   -0.11%     
==========================================
  Files         286      286              
  Lines       30801    30801              
==========================================
- Hits        25766    25734      -32     
- Misses       3980     4002      +22     
- Partials     1055     1065      +10     
Files Coverage Δ
internal/tcp_keepalive_unix.go 100.00% <100.00%> (ø)

... and 17 files with indirect coverage changes

@@ -1,3 +1,5 @@
//go:build unix
Copy link
Member

Choose a reason for hiding this comment

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

This does seem to work for me. Without any build constraint:

$ GOOS=illumos go build ./internal
$ GOOS=android go build ./internal
$ GOOS=ios go build ./internal
$ GOOS=windows go build ./internal
# google.golang.org/grpc/internal
internal/tcp_keepalive.go:48:10: undefined: unix.SetsockoptInt
internal/tcp_keepalive.go:48:38: undefined: unix.SOL_SOCKET
internal/tcp_keepalive.go:48:55: undefined: unix.SO_KEEPALIVE

And with the build constraint, the windows build passes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't realize we could build for any OS with GOOS. Thanks.

@easwars easwars merged commit 52baf16 into grpc:master Dec 8, 2023
14 checks passed
easwars added a commit to easwars/grpc-go that referenced this pull request Dec 11, 2023
easwars added a commit that referenced this pull request Dec 11, 2023
@andyliuliming
Copy link

looks like this PR will broke the referencing grpc from bazel.

@dfawley
Copy link
Member

dfawley commented Jan 16, 2024

@andyliuliming can you file an issue and let us know what's wrong so we can try to fix it?

@andyliuliming
Copy link

@andyliuliming can you file an issue and let us know what's wrong so we can try to fix it?

thanks, opened one issue:
#6929
trying to create a mock repo with the minimal repro.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants