Vamsi-klu commented on PR #18483: URL: https://github.com/apache/pinot/pull/18483#issuecomment-4912317188
Verified against the diff - correct and low-risk. nl.altindag.ssl's withIdentityMaterial(InputStream, char[], String) and withTrustMaterial(...) read the stream into a KeyStore synchronously and don't retain it, so scoping each stream in a try-with-resources and closing it right after the with*Material call is equivalent to the old post-build() close on the success path. The win is on the failure path: each stream is now closed before the next potentially-throwing step, so the key-store stream no longer leaks when a later step (the trust-store null-password Precondition, malformed trust store, or builder error in build()) throws - which is the actual FD leak. Swappable ordering is preserved (withSwappable*Material still precedes the corresponding with*Material), and moving openStream() after the swappable call is inert since opening the stream doesn't touch the builder. Matches the try-with-resources idiom already used by TlsUtils.createKeyManagerFactory and createTrustManagerFact ory in the same package. One coverage note: RenewableTlsUtilsTest's exception tests exercise the enableAutoRenewal ftp:// validation path, not createSSLFactory's open-then-fail path, so the specific leak being fixed isn't directly asserted. Hard to assert cleanly without spying on openStream(), so a minor gap rather than a blocker. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
