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

Create a new context when renewing a certificate in the background #248

Conversation

ankon
Copy link
Contributor

@ankon ankon commented Aug 8, 2023

The context available to renewDynamicCertificate comes from inside the TLS handshake, and as such may be bounded by the lifespan of the connection. Passing this into a goroutine will lead to problems when the connection ends (and the connection context gets canceled with it) but the goroutine is going to do more I/O on that context.

The context available to `renewDynamicCertificate` comes from inside the TLS handshake, and as such
may be bounded by the lifespan of the connection. Passing this into a goroutine will lead to problems
when the connection ends (and the connection context gets canceled with it) but the goroutine is going
to do more I/O on that context.
@ankon
Copy link
Contributor Author

ankon commented Aug 8, 2023

FWIW, go test -v -short -race ./... fails locally:

=== RUN   TestFindZoneByFqdn/domain_is_a_eTLD
    dnsutil_test.go:172: test 2: expected no error, but got: could not find the start of authority for example.com.ac.: NXDOMAIN
    dnsutil_test.go:175: test 2: expected zone '' but got 'ac.'

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Just seeing this -- exactly the change I was going to make after I read the issue.

Thanks for the patch!

@mholt mholt merged commit 8fac0d0 into caddyserver:master Aug 8, 2023
6 checks passed
@mholt
Copy link
Member

mholt commented Aug 8, 2023

FWIW, go test -v -short -race ./... fails locally:

Yeah, the CertMagic tests currently use live DNS which I kind of hate 😅 I just haven't done the work to mock them up yet. They can be problematic depending on which environments you run them in.

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.

None yet

2 participants