Fix GC finalization cycle in SNI callback implementation #1561
Conversation
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
The SNI callback storage was causing finalization cycle warnings because contexts directly referenced callback boxes through instance variables, while the callbacks themselves could return other contexts, creating circular dependencies that the GC couldn't safely finalize. This fix moves callback box storage from instance variables to a class-level hash keyed by context handle. This breaks the finalization cycle because contexts no longer directly reference the callbacks through their object graph - they only hold primitive handle pointers. When a context is finalized, it looks up and explicitly frees its callback box from the class storage, ensuring proper cleanup without circular dependencies. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
|
Claude finished @carlhoerberg's task —— View job Code Review CompleteTodo List:
Issues FoundMemory leak potential in finalize method (src/stdlib/openssl_sni.cr:94): GC.free(callback_box)Calling Suggested fix: Box(Proc(String, OpenSSL::SSL::Context::Server?)).free(callback_box) |
spuun
approved these changes
Dec 23, 2025
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.
Summary
finalizemethodProblem
The SNI callback implementation was causing "GC Warning: Finalization cycle involving (???)" warnings during specs. This occurred because:
@sni_callback_boxSolution
Changed callback storage from instance variables to class-level hash:
@@sni_callback_boxesstores boxes keyed by context handlefinalizemethod to explicitly free callback boxes and clean up storageTest plan
make test SPEC=spec/sni_spec.cr- 10 examples, 0 failures, 0 GC warningsmake test SPEC=spec/stdlib/openssl_keylog_spec.cr- 0 GC warnings🤖 Generated with Claude Code