Jens-G commented on PR #3736: URL: https://github.com/apache/thrift/pull/3736#issuecomment-5468776783
### Code review Re-reviewed after the force-push. The revised patch resolves the earlier finding: moving `!defined(LIBRESSL_VERSION_NUMBER)` from the inner guard to the outer one means LibreSSL now falls through to the `#else` and calls `ERR_remove_state(0)`, so it gets real per-thread cleanup instead of none. Verified against current LibreSSL sources: it still exports `ERR_remove_state()` (kept deliberately — `err.h` notes "Still used in 2023"), has never shipped `OPENSSL_thread_stop()`, and `LIBRESSL_VERSION_NUMBER` comes in via the `<openssl/opensslv.h>` already included at line 47. The outer-guard placement also matches the AWS-LC precedent in `61080c673f`, while correctly leaving BoringSSL/AWS-LC on the inner guard, since those two have no `ERR_remove_state()` at all. Found 2 issues: 1. Merge sequencing: this fix now depends on the `ERR_remove_state(0)` call that #3752 deletes #3752 removes `ERR_remove_state(0)` from both of these `#else` branches on the grounds that it is a no-op since OpenSSL 1.1 — true for mainline, but after this PR LibreSSL is the branch's main consumer. If #3752 lands after #3736, LibreSSL silently goes back to no cleanup at all, which is exactly the defect the force-push just fixed. Worth sequencing the two, or keeping a LibreSSL carve-out in whichever lands second. (#3761 removes `ERR_remove_state` only from the c_glib file, so it does not collide here.) https://github.com/apache/thrift/blob/45364c031a18e4f63035a7972bd6351f3eb3ee55/lib/cpp/src/thrift/transport/TSSLSocket.cpp#L159-L172 https://github.com/apache/thrift/blob/45364c031a18e4f63035a7972bd6351f3eb3ee55/lib/cpp/src/thrift/transport/TSSLSocket.cpp#L407-L420 2. Commit message is missing the `Client:` trailer ([AGENTS.md](https://github.com/apache/thrift/blob/45364c031a18e4f63035a7972bd6351f3eb3ee55/AGENTS.md#L33-L38) says "Commit message includes affected `Client:` languages") The commit is a bare subject line with no body; expected `Client: cpp`. https://github.com/apache/thrift/blob/45364c031a18e4f63035a7972bd6351f3eb3ee55/AGENTS.md#L33-L38 🤖 Generated with [Claude Code](https://claude.ai/code) <sub>- If this code review was useful, please react with 👍. Otherwise, react with 👎.</sub> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
