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

Clone TLS conf in newClient #3400

Merged
merged 1 commit into from
May 12, 2022
Merged

Clone TLS conf in newClient #3400

merged 1 commit into from
May 12, 2022

Conversation

betamos
Copy link
Contributor

@betamos betamos commented May 3, 2022

Fixes #3394

  • I searched for other potential places in non-tests where this was happening, and this is the only one I found. In other places, it's appropriately cloned already.
  • I took a look at the unit tests, but it seems out of place with a specific client test in that file given the harness (BeforeEach) and the other tests. Also it would be very fragile to changes, and likely not catch other instances. I'd recommend taking a look at running integration tests with -race in case you're not already doing that. Or let me know if you already a unit test approach in mind.

Obligatory thanks for maintaining this amazing library.

@google-cla
Copy link

google-cla bot commented May 3, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

Copy link
Member

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you @betamos!

Can you please sign the CLA? Then we can merge the PR.

@codecov
Copy link

codecov bot commented May 3, 2022

Codecov Report

Merging #3400 (b8f0373) into master (5dedb7e) will decrease coverage by 0.01%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master    #3400      +/-   ##
==========================================
- Coverage   85.36%   85.36%   -0.01%     
==========================================
  Files         135      135              
  Lines        9928     9930       +2     
==========================================
+ Hits         8475     8476       +1     
  Misses       1068     1068              
- Partials      385      386       +1     
Impacted Files Coverage Δ
client.go 78.91% <50.00%> (-0.46%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5dedb7e...b8f0373. Read the comment docs.

@betamos
Copy link
Contributor Author

betamos commented May 3, 2022

Can you please sign the CLA?

I did earlier today. Verified that email matches commit and username is correct. Is it possible to re-run the workflow? Otherwise, I don't know what could be the issue.

@marten-seemann
Copy link
Member

@lucas-clemente Can you please check? Looks like the CLA bot is misbehaving (again).

@betamos
Copy link
Contributor Author

betamos commented May 7, 2022

I was able to retrigger the CLA check myself, there was a link at the bottom that I missed the first time around.

Good to go now :)

@marten-seemann marten-seemann merged commit d39a33f into quic-go:master May 12, 2022
nmldiegues pushed a commit to chungthuang/quic-go that referenced this pull request Jun 6, 2022
@MarcoPolo MarcoPolo mentioned this pull request Jul 7, 2022
41 tasks
sudarshan-reddy pushed a commit to sudarshan-reddy/quic-go that referenced this pull request Aug 9, 2022
@ajnavarro ajnavarro mentioned this pull request Aug 24, 2022
72 tasks
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.

Race condition in DialContext
2 participants