bryancall commented on code in PR #13844: URL: https://github.com/apache/trafficserver/pull/13844#discussion_r4296248362
########## 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: I'm leaving this as is. The tree uses Catch2 3.9, which has no option to repeat a test case inside one process, and this test has no SECTIONs, so the body runs once per run of test_net. Even if it did run twice, the second hook would be called under the same lock, and the test would still see one caller at a time. ########## 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: I'm leaving this as is. "and some of those are not event threads" is already correct, and "and some of which are not event threads" would not be. ########## 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: I'm leaving this as is. loadSecret() runs once per certificate or key name while certificates load, and each call then reads and parses a file, so taking an uncontended lock around an empty hook list costs nothing measurable. Checking curHook first would add a branch to save that. -- 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]
