handshake: skip building event data nobody will see - #406
Merged
Conversation
GetCertificateWithContext emits tls_get_certificate as its first statement, so its data is assembled on every handshake. emit() returns right away when OnEvent is nil, but Go evaluates arguments first, so the map and the ClientHello copy are built either way. Add an optional HasEventSubscribersFunc to Config so an embedder can say whether anything is subscribed to an event, and consult it through eventHasSubscriber() before building the data. Leaving the field unset keeps today's behavior: with no way to ask, every event is assumed to be observed. A nil OnEvent falls out of the same check. Only the handshake needs this. The other emit calls happen per certificate, where the data costs less than tracking whether it is wanted. Benchmarks are included; measurements are in the pull request.
u5surf
marked this pull request as ready for review
September 9, 2026 03:35
steadytao
requested changes
Sep 9, 2026
Rename HasEventSubscribersFunc to ShouldEmitFunc and document the contract it actually carries: returning false skips the OnEvent call entirely, so it has to account for everything OnEvent does, not just subscribed handlers. Caddy writes a debug line for every event whether or not anything is subscribed, and a predicate that answered only about subscribers would silence it. Inherit the predicate from Default only alongside the handler it was written for. A caller that supplies its own OnEvent and leaves the predicate nil would otherwise pick up an unrelated predicate from Default and have its events suppressed. Covered by a regression test.
steadytao
approved these changes
Sep 9, 2026
steadytao
left a comment
Member
There was a problem hiding this comment.
Thank you. It does lgtm but ill let Matt or Francis touch seeing as I am not too familiar with this codebase -- @mholt @francislavoie
francislavoie
approved these changes
Sep 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #405.
Problem
GetCertificateWithContextemitstls_get_certificateas its first statement:emitreturns immediately whenOnEventis nil, but Go evaluates arguments first, so the map and theserializableClientHellocopy are built on every handshake whether or not anything can receive them.Change
An optional
Config.ShouldEmitFunclets an embedder say whether an event is worth emitting. It is consulted through a small helper:Four things worth calling out:
Leaving the field unset keeps today's behavior exactly. The middle branch is the one every existing user takes: with no way to ask, every event is emitted. Nothing changes for anyone who does not opt in.
The predicate only comes along with the handler it was written for.
newWithCacheinheritsShouldEmitFuncfromDefaultonly when it also inheritedOnEvent; a caller that brings its own handler and no predicate keeps a nil one, rather than picking up an unrelated predicate that knows nothing about it.TestShouldEmitFuncInheritedWithOnEventcovers the three combinations.The check has to be at the call site.
emitalready returns early on a nilOnEvent, but by the time it is entered its arguments exist. That is the whole problem, and it is why the guard cannot live insideemit. The helper's doc comment says so, to keep it from being "simplified" away later.Only the handshake is guarded. The other twelve
emitcalls happen per certificate — caching, issuance, renewal, OCSP — where the data costs less than deciding whether to build it. Sprinkling the guard there would be noise.Benchmarks
GetCertificateagainst a cached certificate, with anOnEventhandler installed in both cases (Apple M2):GetCertificateEventSubscribedGetCertificateEventUnsubscribedThe 672 bytes and 7 allocations are exactly what building the event data costs; measured on its own it is 169 ns/op, 672 B/op, 7 allocs/op. This is not a full handshake — no TLS crypto, no I/O — so it isolates the lookup step.
Tests
TestShouldEmitcovers the five states of the two hooks, including that a nilOnEventoutweighs a subscriber claim.TestGetCertificateSkipsUnobservedEventchecks the behavior throughGetCertificatefor subscribed, unsubscribed, and no-way-to-ask.TestGetCertificateHonorsEventAbortpins down that a subscribed handler can still abort a handshake, which is the thing it would be worst to break silently.On the Caddy side
Caddy always installs an
OnEventhook, so the nil case alone would not help it. Its events app already keeps subscriptions per event name — caddyserver/caddy#7997 uses exactly that to shortcut dispatch — soShouldEmitFuncwould be a couple of lines there. I'll send that once this lands and Caddy picks up a release.Assistance disclosure: I investigated this with Claude Code (Claude Opus 5), which read the code paths, wrote the patch, the benchmarks and the tests, and ran the test suite. I directed the investigation, reviewed the change, and vetted it for correctness.