Copilot commented on code in PR #13844: URL: https://github.com/apache/trafficserver/pull/13844#discussion_r4286745247
########## src/iocore/net/unit_tests/test_SSLSecret.cc: ########## @@ -0,0 +1,98 @@ +/** @file + + Catch based unit tests for SSLSecret + + @section license License + + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + */ + +#include "../P_SSLSecret.h" + +#include "api/LifecycleAPIHooks.h" +#include "ts/InkAPIPrivateIOCore.h" + +#include <catch2/catch_test_macros.hpp> + +#include <atomic> +#include <chrono> +#include <condition_variable> +#include <mutex> +#include <string> +#include <thread> +#include <vector> Review Comment: `std::max` is used later in this file, but `<algorithm>` is not included. Some toolchains won’t compile this reliably via transitive includes; add `#include <algorithm>`. ########## src/iocore/net/unit_tests/test_SSLSecret.cc: ########## @@ -0,0 +1,98 @@ +/** @file + + Catch based unit tests for SSLSecret + + @section license License + + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + */ + +#include "../P_SSLSecret.h" + +#include "api/LifecycleAPIHooks.h" +#include "ts/InkAPIPrivateIOCore.h" + +#include <catch2/catch_test_macros.hpp> + +#include <atomic> +#include <chrono> +#include <condition_variable> +#include <mutex> +#include <string> +#include <thread> +#include <vector> + +namespace +{ +// Only count hook calls made by this test. The hook stays registered for the +// rest of the test binary, since lifecycle hooks cannot be removed. +std::atomic<bool> hook_armed{false}; + +std::mutex hook_mutex; +std::condition_variable hook_cv; +int callers_in_hook{0}; +int max_callers_in_hook{0}; + +int +count_concurrent_callers(TSCont /* cont ATS_UNUSED */, TSEvent /* event ATS_UNUSED */, void * /* edata ATS_UNUSED */) +{ + if (!hook_armed) { + return 0; + } + std::unique_lock lock(hook_mutex); + ++callers_in_hook; + max_callers_in_hook = std::max(max_callers_in_hook, callers_in_hook); + hook_cv.notify_all(); + // Linger until a second caller arrives, or give up. If the hook is not + // serialized, the other threads are already waiting to enter it. + hook_cv.wait_for(lock, std::chrono::milliseconds(250), [] { return callers_in_hook > 1; }); + --callers_in_hook; + return 0; +} +} // end anonymous namespace + +TEST_CASE("SSL_SECRET hooks run one caller at a time") +{ + // Parallel ssl_multicert loading calls getOrLoadSecret() from several plain + // std::threads. A plugin's secret hook must still see one call at a time. + g_lifecycle_hooks->append(TS_LIFECYCLE_SSL_SECRET_HOOK, new INKContInternal(count_concurrent_callers, nullptr)); + hook_armed = true; Review Comment: This registers a lifecycle hook inside the test body, but lifecycle hooks cannot be removed. If the test binary is run with Catch2 repetition (e.g., `--repeat`) or the test is executed multiple times in-process, the hook will be appended repeatedly and permanently, potentially inflating runtime and affecting observability. Consider registering the hook exactly once (e.g., via `std::once_flag`) and resetting only the counters/arming flag per test run. ########## doc/developer-guide/api/functions/TSLifecycleHookAdd.en.rst: ########## @@ -114,6 +114,10 @@ Types This hook gives the plugin a chance to load the certificate or key from an alternative source and set via the :func:`TSSslSecretSet` API. If there is no plugin override, the certificate or key will be loaded from disk and the secret name will be interpreted as a file path. + |TS| may load certificates on several threads at once, and some of those are not event threads, so a mutex on the Review Comment: Minor grammar: change 'and some of those are not event threads' to 'and some of which are not event threads' to read more clearly. ########## src/iocore/net/unit_tests/test_SSLSecret.cc: ########## @@ -0,0 +1,98 @@ +/** @file + + Catch based unit tests for SSLSecret + + @section license License + + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + */ + +#include "../P_SSLSecret.h" + +#include "api/LifecycleAPIHooks.h" +#include "ts/InkAPIPrivateIOCore.h" + +#include <catch2/catch_test_macros.hpp> + +#include <atomic> +#include <chrono> +#include <condition_variable> +#include <mutex> +#include <string> +#include <thread> +#include <vector> + +namespace +{ +// Only count hook calls made by this test. The hook stays registered for the +// rest of the test binary, since lifecycle hooks cannot be removed. +std::atomic<bool> hook_armed{false}; + +std::mutex hook_mutex; +std::condition_variable hook_cv; +int callers_in_hook{0}; +int max_callers_in_hook{0}; + +int +count_concurrent_callers(TSCont /* cont ATS_UNUSED */, TSEvent /* event ATS_UNUSED */, void * /* edata ATS_UNUSED */) +{ + if (!hook_armed) { + return 0; + } + std::unique_lock lock(hook_mutex); + ++callers_in_hook; + max_callers_in_hook = std::max(max_callers_in_hook, callers_in_hook); + hook_cv.notify_all(); + // Linger until a second caller arrives, or give up. If the hook is not + // serialized, the other threads are already waiting to enter it. + hook_cv.wait_for(lock, std::chrono::milliseconds(250), [] { return callers_in_hook > 1; }); + --callers_in_hook; + return 0; +} +} // end anonymous namespace + +TEST_CASE("SSL_SECRET hooks run one caller at a time") +{ + // Parallel ssl_multicert loading calls getOrLoadSecret() from several plain + // std::threads. A plugin's secret hook must still see one call at a time. + g_lifecycle_hooks->append(TS_LIFECYCLE_SSL_SECRET_HOOK, new INKContInternal(count_concurrent_callers, nullptr)); + hook_armed = true; + + constexpr int num_threads = 4; + SSLSecret secrets; + std::atomic<int> waiting{num_threads}; + std::vector<std::thread> threads; + for (int i = 0; i < num_threads; ++i) { + threads.emplace_back([&secrets, &waiting, i]() { + --waiting; + while (waiting > 0) { + std::this_thread::yield(); + } + // Neither file exists, so the secret is never cached and the hook runs. + std::string cert_data; + std::string key_data; + secrets.getOrLoadSecret("/nonexistent/cert" + std::to_string(i) + ".pem", "/nonexistent/key" + std::to_string(i) + ".pem", + cert_data, key_data); Review Comment: The test relies on `/nonexistent/...` not existing. That’s usually true, but it’s not guaranteed across all test environments/containers (or if a system happens to have that path). To make the test deterministic, generate a per-run unique path under a temp directory (or otherwise guarantee non-existence) and verify it doesn’t exist before calling `getOrLoadSecret()`. ########## src/iocore/net/SSLSecret.cc: ########## @@ -66,9 +73,12 @@ SSLSecret::loadSecret(const std::string &name1, const std::string &name2, std::s secret_name.cert_name_len = name1.size(); secret_name.key_name = name2.data(); secret_name.key_name_len = name2.size(); - while (curHook) { - curHook->blocking_invoke(TS_EVENT_SSL_SECRET, &secret_name); - curHook = curHook->next(); + { + std::scoped_lock lock(secret_hook_mutex); + while (curHook) { + curHook->blocking_invoke(TS_EVENT_SSL_SECRET, &secret_name); + curHook = curHook->next(); + } Review Comment: The mutex is acquired even when there are no registered hooks (`curHook == nullptr`). If this path is hot during multi-cert loading, you can avoid unnecessary lock/unlock by checking `curHook` first and only locking when at least one hook will actually be invoked. -- 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]
