Copilot commented on code in PR #13454:
URL: https://github.com/apache/trafficserver/pull/13454#discussion_r3679101416
##########
src/proxy/ReverseProxy.cc:
##########
@@ -49,12 +51,44 @@ Ptr<ProxyMutex> reconfig_mutex;
DbgCtl dbg_ctl_url_rewrite{"url_rewrite"};
+// Steers UrlRewriteDeleter to inline-delete; see shutdown_url_rewrite().
Review Comment:
Comment says shutdown steers the deleter to "inline-delete", but the deleter
actually returns early and intentionally leaks the table once shutdown starts.
This is misleading for future maintainers trying to reason about teardown
behavior during shutdown.
##########
include/tsutil/AtomicSharedPtr.h:
##########
@@ -0,0 +1,84 @@
+/** @file
+
+ Atomic wrapper around std::shared_ptr with the C++20
+ std::atomic<std::shared_ptr<T>> API.
+
+ @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.
+ */
+
+#pragma once
+
+#include <atomic>
+#include <memory>
+
+// Use the C++20 std::atomic<std::shared_ptr<T>> specialization when the
+// standard library provides it, otherwise fall back to the pre-C++20
+// std::atomic_*_explicit free-function overloads on shared_ptr. The
+// fallback exists for libstdc++ < 12 and libc++ < 14, which predate the
+// specialization. When those toolchains are no longer supported, delete
+// the #else branch and the surrounding #if; call sites do not change.
+#if defined(__cpp_lib_atomic_shared_ptr) && __cpp_lib_atomic_shared_ptr >=
201711L
+
+template <class T> using AtomicSharedPtr = std::atomic<std::shared_ptr<T>>;
+
+#else
+
+// Belt-and-suspenders: on the toolchains that take this branch (libstdc++
+// < 12, libc++ < 16) the free-function overloads are not yet marked
+// [[deprecated]], so the suppression below is usually a no-op. It
+// matters only if someone forces the fallback on a modern library (e.g.
+// -D__cpp_lib_atomic_shared_ptr=0) or compiles against a library that
+// ships the deprecation markers ahead of the specialization.
Review Comment:
AtomicSharedPtr fallback comment contradicts itself about which libc++
versions lack the std::atomic<std::shared_ptr<T>> specialization (earlier says
libc++ < 14, later says libc++ < 16). Please make these consistent to avoid
confusion when toolchain support is updated.
##########
include/proxy/ReverseProxy.h:
##########
@@ -61,4 +63,10 @@ bool reloadUrlRewrite(ConfigContext ctx);
bool urlRewriteVerify();
void init_remap_volume_host_records();
-int url_rewrite_CB(const char *name, RecDataT data_type, RecData data, void
*cookie);
+
+// Synchronously drops rewrite_table. Call from a Continuation context
+// before TSSystemState::shut_down_event_system() so plugin doneInstance()
+// has this_ethread() for TSMutexLock.
Review Comment:
The comment implies shutdown_url_rewrite() ensures plugin doneInstance()
runs while this_ethread() is valid, but shutdown_url_rewrite() only drops the
global reference. If other shared_ptr holders exist, teardown will be
suppressed (leaked) after shutdown starts, so doneInstance() may not run.
Please clarify the comment to match the implementation’s behavior.
--
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]
