You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "bneradt (via GitHub)" <gi...@apache.org> on 2026/08/12 01:06:05 UTC
[PR] Fix records_yaml startup lookup race [trafficserver]
bneradt opened a new pull request, #13538:
URL: https://github.com/apache/trafficserver/pull/13538
The records_yaml AuTest intermittently reported that
proxy.config.diags.output.note was missing when a configuration
callback ran while startup was still registering metrics.
Metrics::find() returned the then-current end iterator for a miss, but
RecLookupRecord() compared it with a new end iterator. An intervening
registration made them differ and misread the missing string record as
a numeric metric.
This patch addresses the race by using the direct metrics lookup API,
which reports a miss without comparing two moving end positions. It also
adds a concurrent registration test that exercises string record lookups
during metric creation.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix records_yaml startup lookup race [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #13538:
URL: https://github.com/apache/trafficserver/pull/13538#discussion_r3808296741
##########
src/records/unit_tests/test_RecRegister.cc:
##########
@@ -87,3 +92,43 @@ TEST_CASE("RecRegisterStat - Type Dispatch", "[librecords][RecStat]")
REQUIRE(value == 500);
}
}
+
+TEST_CASE("RecLookupRecord - Concurrent metric registration", "[librecords][RecLookup]")
+{
+ constexpr char record_name[] = "proxy.test.concurrent.string_value";
+ constexpr char record_value[] = "stable";
+
+ REQUIRE(RecRegisterConfigString(RECT_CONFIG, record_name, record_value, RECU_DYNAMIC, RECC_NULL, nullptr, REC_SOURCE_NULL) ==
+ REC_ERR_OKAY);
+
+ std::atomic<bool> start{false};
+ std::atomic<bool> finished{false};
+ std::thread register_metrics([&]() {
+ while (!start.load(std::memory_order_acquire)) {
+ std::this_thread::yield();
+ }
+
+ for (int i = 0; i < 100000; ++i) {
+ auto metric_name = std::string{"proxy.process.test.concurrent_metric_registration."} + std::to_string(i);
+
+ ts::Metrics::Counter::create(metric_name);
+ }
+ finished.store(true, std::memory_order_release);
+ });
+
+ bool all_lookups_succeeded = true;
+ size_t lookup_count = 0;
+
+ start.store(true, std::memory_order_release);
+ while (!finished.load(std::memory_order_acquire)) {
+ if (RecGetRecordStringAlloc(record_name) != record_value) {
+ all_lookups_succeeded = false;
+ break;
+ }
+ ++lookup_count;
+ }
Review Comment:
Changed to a do/while loop so at least one lookup always occurs, and switched to unnamed Counter::createSpan(1) allocations to avoid the start spin and retaining 100,000 metric names.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix records_yaml startup lookup race [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #13538:
URL: https://github.com/apache/trafficserver/pull/13538#discussion_r3766510123
##########
src/records/unit_tests/test_RecRegister.cc:
##########
@@ -17,11 +17,16 @@
or implied. See the License for the specific language governing permissions and limitations under
the License.
*/
+#include <atomic>
+#include <string>
+#include <thread>
Review Comment:
Nit: it's better to include standard headers after local project headers to expose missing includes quickly.
##########
src/records/unit_tests/test_RecRegister.cc:
##########
@@ -87,3 +92,43 @@ TEST_CASE("RecRegisterStat - Type Dispatch", "[librecords][RecStat]")
REQUIRE(value == 500);
}
}
+
+TEST_CASE("RecLookupRecord - Concurrent metric registration", "[librecords][RecLookup]")
+{
+ constexpr char record_name[] = "proxy.test.concurrent.string_value";
+ constexpr char record_value[] = "stable";
+
+ REQUIRE(RecRegisterConfigString(RECT_CONFIG, record_name, record_value, RECU_DYNAMIC, RECC_NULL, nullptr, REC_SOURCE_NULL) ==
+ REC_ERR_OKAY);
+
+ std::atomic<bool> start{false};
+ std::atomic<bool> finished{false};
+ std::thread register_metrics([&]() {
+ while (!start.load(std::memory_order_acquire)) {
+ std::this_thread::yield();
+ }
+
+ for (int i = 0; i < 100000; ++i) {
+ auto metric_name = std::string{"proxy.process.test.concurrent_metric_registration."} + std::to_string(i);
+
+ ts::Metrics::Counter::create(metric_name);
+ }
+ finished.store(true, std::memory_order_release);
+ });
+
+ bool all_lookups_succeeded = true;
+ size_t lookup_count = 0;
+
+ start.store(true, std::memory_order_release);
+ while (!finished.load(std::memory_order_acquire)) {
+ if (RecGetRecordStringAlloc(record_name) != record_value) {
+ all_lookups_succeeded = false;
+ break;
+ }
+ ++lookup_count;
+ }
Review Comment:
This loop can run 0 times in theory, which will fail the test. What was the intent here?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix records_yaml startup lookup race [trafficserver]
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on PR #13538:
URL: https://github.com/apache/trafficserver/pull/13538#issuecomment-5432361941
Cherry-picked to the 10.2.x branch as 43e3d233d8c412ffb48c87a2041e97aefae749b6 for the 10.2.1 release.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix records_yaml startup lookup race [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt merged PR #13538:
URL: https://github.com/apache/trafficserver/pull/13538
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix records_yaml startup lookup race [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #13538:
URL: https://github.com/apache/trafficserver/pull/13538#discussion_r3808291657
##########
src/records/unit_tests/test_RecRegister.cc:
##########
@@ -17,11 +17,16 @@
or implied. See the License for the specific language governing permissions and limitations under
the License.
*/
+#include <atomic>
+#include <string>
+#include <thread>
Review Comment:
Done. Project headers now precede the standard-library headers.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org