Skip to content

fix: wait for HTTP server serve() termination #348

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

Merged
merged 3 commits into from
Feb 27, 2025

Conversation

rogercoll
Copy link
Contributor

Fixes #347

@rogercoll rogercoll requested a review from a team as a code owner February 11, 2025 14:28
Copy link

codecov bot commented Feb 11, 2025

Codecov Report

Attention: Patch coverage is 65.00000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 78.46%. Comparing base (d1dbd0c) to head (cb710d3).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
server/serverimpl.go 65.00% 6 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #348      +/-   ##
==========================================
+ Coverage   78.35%   78.46%   +0.11%     
==========================================
  Files          25       25              
  Lines        2402     2419      +17     
==========================================
+ Hits         1882     1898      +16     
- Misses        412      413       +1     
  Partials      108      108              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tigrannajaryan
Copy link
Member

Please update doc-comment of OpAMPServer.Stop() to explain that it waits until the server socket is released.

@rogercoll
Copy link
Contributor Author

looks like there is an unrelated flaky test on the examples:
2025/02/13 14:49:28.332879 [OPAMP] Could not load TLS config, working without TLS: open ../../certs/certs/ca.cert.pem: no such file or directory

@rogercoll
Copy link
Contributor Author

related data race issue #256

@tigrannajaryan
Copy link
Member

related data race issue #256

I rerun the build a couple times and it fails. Perhaps this PR changed timings and the race is now encountered more consistently. Regardless of the reason I can't merge the PR if the build does not pass. We need to fix #256 first.

@rogercoll
Copy link
Contributor Author

@tigrannajaryan The race condition test is now green after merging main. There are some changes on the files related to formatting which I think it would be better to tackle in a separate PR: #356

@tigrannajaryan
Copy link
Member

@tigrannajaryan The race condition test is now green after merging main. There are some changes on the files related to formatting which I think it would be better to tackle in a separate PR: #356

Excellent! Let's get #356 merged and then we will merge this one. Thank you for your patience and for going the extra mile.

@rogercoll
Copy link
Contributor Author

@tigrannajaryan The race condition test is now green after merging main. There are some changes on the files related to formatting which I think it would be better to tackle in a separate PR: #356

Excellent! Let's get #356 merged and then we will merge this one. Thank you for your patience and for going the extra mile.

Thanks for your review! Rebased 👍

@tigrannajaryan tigrannajaryan merged commit d27bb62 into open-telemetry:main Feb 27, 2025
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[HTTP] Server shutdown completes before serve() terminates
2 participants