ezelkow1 commented on PR #13844: URL: https://github.com/apache/trafficserver/pull/13844#issuecomment-6278793398
Thanks for picking this up. I wrote #12998 and missed that the SSL_SECRET hooks would start running concurrently. The `std::thread` / `this_ethread()` gap means no plugin could have guarded this itself, so the fix belongs in core. I mainly checked whether this lock gives back the parallelism from #12998. It doesn't. The lock only covers the hook loop in `loadSecret()`. The expensive per-cert work (`_prep_ssl_ctx` / `init_server_ssl_ctx`, PEM parsing, `SSL_CTX` setup) and the disk reads in `loadFile()` all stay outside it. With no secret plugin loaded it's an uncontended lock per new secret name. With an in-memory plugin like `ssl_secret_load_test` the serialized part is tiny next to cert parsing. Only plugins that block inside the hook (synchronous fetches from a remote secret store) end up serialized, and those were racing before this change anyway. Going recursive was the right call given the `TSSslSecretSet()` → `updateCTX()` re-entry. A few things: 1. **Backport:** #12998 went to 10.2.x in #13043, so 10.2.x has the same race. Can we add the Backport label? 2. **Cross-thread deadlock:** The docs now cover a plugin calling `TSSslSecretUpdate()` from its own thread while holding a lock its hook also takes. That's a new way to deadlock that didn't exist before. It's probably worth a line in the 10.2.x release notes, since plugins that worked before could hang on reload after this. 3. **Possible follow-up, not a blocker:** if network-backed secret plugins become common, we could let a plugin mark its hook as thread-safe and skip the global lock, so it still gets parallel loading. Correct by default is the right choice for now. LGTM. -- 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]
