Skip to content

Commit c106edd

Browse files
yashyktcopybara-github
authored andcommitted
[http2] Fix data race on tcp_tracer (grpc#39015)
Remove tcp_tracer in chttp2_transport since it's unused and buggy. Fix for TSAN failure (similar to grpc#38728) - https://btx.cloud.google.com/invocations/97eb07c8-cf0c-4c26-a0c4-e7417adb1f23/targets/%2F%2Ftest%2Fcpp%2Fext%2Fotel:otel_tracing_test@poller%3Dpoll;config=1a7b8f82b5a4a85323c9dfbac159b038ad684a9dcca8ab0ce8fe99aa87dd2da2/log ``` WARNING: ThreadSanitizer: data race (pid=15) Read of size 8 at 0x726c00001588 by thread T44: #0 GetContext /proc/self/cwd/./src/core/lib/resource_quota/arena.h:296:9 (liblibgrpc_Utransport_Uchttp2.so+0x30af6e) #1 (anonymous namespace)::TcpTracerIfSampled(grpc_chttp2_stream*) /proc/self/cwd/src/core/ext/transport/chttp2/transport/chttp2_transport.cc:243:17 (liblibgrpc_Utransport_Uchttp2.so+0x30af6e) cpp-pm#2 perform_stream_op_locked(void*, absl::lts_20240722::Status) /proc/self/cwd/src/core/ext/transport/chttp2/transport/chttp2_transport.cc:1655:19 (liblibgrpc_Utransport_Uchttp2.so+0x3014aa) cpp-pm#3 grpc_combiner_continue_exec_ctx() /proc/self/cwd/src/core/lib/iomgr/combiner.cc:216:5 (liblibexec_Uctx.so+0x10d7e) hunter-packages#4 grpc_core::ExecCtx::Flush() /proc/self/cwd/src/core/lib/iomgr/exec_ctx.cc:77:17 (liblibexec_Uctx.so+0x16e45) grpc#5 queue_offload(grpc_core::Combiner*)::$_0::operator()() const /proc/self/cwd/src/core/lib/iomgr/combiner.cc:165:14 (liblibexec_Uctx.so+0x13092) grpc#6 void std::__invoke_impl(std::__invoke_other, queue_offload(grpc_core::Combiner*)::$_0&) /usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/invoke.h:60:14 (liblibexec_Uctx.so+0x13005) grpc#7 std::__invoke_result::type std::__invoke(queue_offload(grpc_core::Combiner*)::$_0&) /usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/invoke.h:95:14 (liblibexec_Uctx.so+0x12fb5) grpc#8 std::invoke_result::type std::invoke(queue_offload(grpc_core::Combiner*)::$_0&) /usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/functional:81:14 (liblibexec_Uctx.so+0x12f65) grpc#9 void absl::lts_20240722::internal_any_invocable::InvokeR(queue_offload(grpc_core::Combiner*)::$_0&) /proc/self/cwd/external/com_google_absl/absl/functional/internal/any_invocable.h:132:3 (liblibexec_Uctx.so+0x12f05) grpc#10 void absl::lts_20240722::internal_any_invocable::LocalInvoker(absl::lts_20240722::internal_any_invocable::TypeErasedState*) /proc/self/cwd/external/com_google_absl/absl/functional/internal/any_invocable.h:310:10 (liblibexec_Uctx.so+0x12e22) grpc#11 absl::lts_20240722::internal_any_invocable::Impl::operator()() /proc/self/cwd/external/com_google_absl/absl/functional/internal/any_invocable.h:868:1 (libsrc_Score_Slibping_Ucallbacks.so+0x260bf) grpc#12 grpc_event_engine::experimental::SelfDeletingClosure::Run() /proc/self/cwd/./src/core/lib/event_engine/common_closures.h:54:5 (libsrc_Score_Slibevent_Uengine_Uthread_Upool.so+0x501bd) grpc#13 grpc_event_engine::experimental::WorkStealingThreadPool::ThreadState::Step() /proc/self/cwd/src/core/lib/event_engine/thread_pool/work_stealing_thread_pool.cc:524:14 (libsrc_Score_Slibevent_Uengine_Uthread_Upool.so+0x4a949) grpc#14 grpc_event_engine::experimental::WorkStealingThreadPool::ThreadState::ThreadBody() /proc/self/cwd/src/core/lib/event_engine/thread_pool/work_stealing_thread_pool.cc:487:10 (libsrc_Score_Slibevent_Uengine_Uthread_Upool.so+0x4a013) grpc#15 grpc_event_engine::experimental::WorkStealingThreadPool::WorkStealingThreadPoolImpl::StartThread()::$_0::operator()(void*) const /proc/self/cwd/src/core/lib/event_engine/thread_pool/work_stealing_thread_pool.cc:257:17 (libsrc_Score_Slibevent_Uengine_Uthread_Upool.so+0x4b489) grpc#16 grpc_event_engine::experimental::WorkStealingThreadPool::WorkStealingThreadPoolImpl::StartThread()::$_0::__invoke(void*) /proc/self/cwd/src/core/lib/event_engine/thread_pool/work_stealing_thread_pool.cc:255:7 (libsrc_Score_Slibevent_Uengine_Uthread_Upool.so+0x4b429) grpc#17 grpc_core::(anonymous namespace)::ThreadInternalsPosix::ThreadInternalsPosix(char const*, void (*)(void*), void*, bool*, grpc_core::Thread::Options const&)::'lambda'(void*)::operator()(void*) const /proc/self/cwd/src/core/util/posix/thd.cc:145:11 (liblibgpr.so+0x1c2e0) grpc#18 grpc_core::(anonymous namespace)::ThreadInternalsPosix::ThreadInternalsPosix(char const*, void (*)(void*), void*, bool*, grpc_core::Thread::Options const&)::'lambda'(void*)::__invoke(void*) /proc/self/cwd/src/core/util/posix/thd.cc:115:9 (liblibgpr.so+0x1c109) Previous write of size 8 at 0x726c00001588 by thread T47: #0 void grpc_core::Arena::SetContext(grpc_core::CallTracerInterface*) /proc/self/cwd/./src/core/lib/resource_quota/arena.h:305:10 (liblibgrpc_Uclient_Uchannel.so+0x5ac294) #1 grpc_core::(anonymous namespace)::CreateCallAttemptTracer(grpc_core::Arena*, bool) /proc/self/cwd/src/core/client_channel/client_channel_filter.cc:2388:10 (liblibgrpc_Uclient_Uchannel.so+0x58403c) cpp-pm#2 grpc_core::ClientChannelFilter::LoadBalancedCall::LoadBalancedCall(grpc_core::ClientChannelFilter*, grpc_core::Arena*, absl::lts_20240722::AnyInvocable, bool) /proc/self/cwd/src/core/client_channel/client_channel_filter.cc:2402:11 (liblibgrpc_Uclient_Uchannel.so+0x583a98) cpp-pm#3 grpc_core::ClientChannelFilter::FilterBasedLoadBalancedCall::FilterBasedLoadBalancedCall(grpc_core::ClientChannelFilter*, grpc_call_element_args const&, grpc_polling_entity*, grpc_closure*, absl::lts_20240722::AnyInvocable, bool) /proc/self/cwd/src/core/client_channel/client_channel_filter.cc:2628:7 (liblibgrpc_Uclient_Uchannel.so+0x5864d4) hunter-packages#4 grpc_core::ClientChannelFilter::FilterBasedLoadBalancedCall* grpc_core::Arena::New, bool&>(grpc_core::ClientChannelFilter*&&, grpc_call_element_args const&, grpc_polling_entity*&, grpc_closure*&, absl::lts_20240722::AnyInvocable&&, bool&) /proc/self/cwd/./src/core/lib/resource_quota/arena.h:181:13 (liblibgrpc_Uclient_Uchannel.so+0x59459d) grpc#5 grpc_core::ClientChannelFilter::CreateLoadBalancedCall(grpc_call_element_args const&, grpc_polling_entity*, grpc_closure*, absl::lts_20240722::AnyInvocable, bool) /proc/self/cwd/src/core/client_channel/client_channel_filter.cc:1142:19 (liblibgrpc_Uclient_Uchannel.so+0x578343) grpc#6 grpc_core::RetryFilter::LegacyCallData::CreateLoadBalancedCall(absl::lts_20240722::AnyInvocable, bool) /proc/self/cwd/src/core/client_channel/retry_filter_legacy_call_data.cc:1635:36 (liblibgrpc_Uclient_Uchannel.so+0x5f3954) grpc#7 grpc_core::RetryFilter::LegacyCallData::CallAttempt::CallAttempt(grpc_core::RetryFilter::LegacyCallData*, bool) /proc/self/cwd/src/core/client_channel/retry_filter_legacy_call_data.cc:129:21 (liblibgrpc_Uclient_Uchannel.so+0x5f2de1) grpc#8 grpc_core::RefCountedPtr grpc_core::MakeRefCounted(grpc_core::RetryFilter::LegacyCallData*&&, bool&) /proc/self/cwd/./src/core/util/ref_counted_ptr.h:369:31 (liblibgrpc_Uclient_Uchannel.so+0x60ad6a) grpc#9 grpc_core::RetryFilter::LegacyCallData::CreateCallAttempt(bool) /proc/self/cwd/src/core/client_channel/retry_filter_legacy_call_data.cc:1644:19 (liblibgrpc_Uclient_Uchannel.so+0x605a9b) grpc#10 grpc_core::RetryFilter::LegacyCallData::StartTransparentRetry(void*, absl::lts_20240722::Status) /proc/self/cwd/src/core/client_channel/retry_filter_legacy_call_data.cc:1937:12 (liblibgrpc_Uclient_Uchannel.so+0x60600b) grpc#11 exec_ctx_run(grpc_closure*) /proc/self/cwd/src/core/lib/iomgr/exec_ctx.cc:43:3 (liblibexec_Uctx.so+0x17536) grpc#12 grpc_core::ExecCtx::Flush() /proc/self/cwd/src/core/lib/iomgr/exec_ctx.cc:74:9 (liblibexec_Uctx.so+0x16e29) grpc#13 grpc_core::ExecCtx::~ExecCtx() /proc/self/cwd/./src/core/lib/iomgr/exec_ctx.h:137:5 (liblibgrpc++_Ubase.so+0x1647cc) grpc#14 grpc_core::WorkSerializer::WorkSerializerImpl::Run() /proc/self/cwd/src/core/util/work_serializer.cc:221:1 (liblibwork_Userializer.so+0x15947) grpc#15 non-virtual thunk to grpc_core::WorkSerializer::WorkSerializerImpl::Run() /proc/self/cwd/src/core/util/work_serializer.cc (liblibwork_Userializer.so+0x15b09) grpc#16 grpc_event_engine::experimental::WorkStealingThreadPool::ThreadState::Step() /proc/self/cwd/src/core/lib/event_engine/thread_pool/work_stealing_thread_pool.cc:524:14 (libsrc_Score_Slibevent_Uengine_Uthread_Upool.so+0x4a949) grpc#17 grpc_event_engine::experimental::WorkStealingThreadPool::ThreadState::ThreadBody() /proc/self/cwd/src/core/lib/event_engine/thread_pool/work_stealing_thread_pool.cc:487:10 (libsrc_Score_Slibevent_Uengine_Uthread_Upool.so+0x4a013) grpc#18 grpc_event_engine::experimental::WorkStealingThreadPool::WorkStealingThreadPoolImpl::StartThread()::$_0::operator()(void*) const /proc/self/cwd/src/core/lib/event_engine/thread_pool/work_stealing_thread_pool.cc:257:17 (libsrc_Score_Slibevent_Uengine_Uthread_Upool.so+0x4b489) grpc#19 grpc_event_engine::experimental::WorkStealingThreadPool::WorkStealingThreadPoolImpl::StartThread()::$_0::__invoke(void*) /proc/self/cwd/src/core/lib/event_engine/thread_pool/work_stealing_thread_pool.cc:255:7 (libsrc_Score_Slibevent_Uengine_Uthread_Upool.so+0x4b429) grpc#20 grpc_core::(anonymous namespace)::ThreadInternalsPosix::ThreadInternalsPosix(char const*, void (*)(void*), void*, bool*, grpc_core::Thread::Options const&)::'lambda'(void*)::operator()(void*) const /proc/self/cwd/src/core/util/posix/thd.cc:145:11 (liblibgpr.so+0x1c2e0) grpc#21 grpc_core::(anonymous namespace)::ThreadInternalsPosix::ThreadInternalsPosix(char const*, void (*)(void*), void*, bool*, grpc_core::Thread::Options const&)::'lambda'(void*)::__invoke(void*) /proc/self/cwd/src/core/util/posix/thd.cc:115:9 (liblibgpr.so+0x1c109) ``` Assumed sequence of events (based on stacktrace) - * The client application creates a new RPC. * A ClientChannelFilter::LoadBalancedCall is created for the first attempt. * A CallAttemptTracer is created for this first attempt and a pointer to this is saved on the arena context. * The first attempt fails without any bytes having been sent on the wire, making this RPC eligible for transparent retries. * A new ClientChannelFilter::LoadBalancedCall created for this new attempt. * A new CallAttemptTracer is created for this second (transparent) attempt and a pointer is saved on the arena context. * Around the same time as a new CallAttemptTracer is created, the chttp2 transport is performing some operations for the first stream (for example, a cancel op). Closes grpc#39015 COPYBARA_INTEGRATE_REVIEW=grpc#39015 from yashykt:FixTcpTracer 7e3746e PiperOrigin-RevId: 739343300
1 parent 9482ae1 commit c106edd

File tree

3 files changed

+3
-20
lines changed

3 files changed

+3
-20
lines changed

src/core/ext/transport/chttp2/transport/chttp2_transport.cc

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,6 @@
104104
#include "src/core/telemetry/call_tracer.h"
105105
#include "src/core/telemetry/stats.h"
106106
#include "src/core/telemetry/stats_data.h"
107-
#include "src/core/telemetry/tcp_tracer.h"
108107
#include "src/core/util/bitset.h"
109108
#include "src/core/util/crash.h"
110109
#include "src/core/util/debug_location.h"
@@ -237,16 +236,6 @@ grpc_core::CallTracerAnnotationInterface* ParentCallTracerIfSampled(
237236
return parent_call_tracer;
238237
}
239238

240-
std::shared_ptr<grpc_core::TcpTracerInterface> TcpTracerIfSampled(
241-
grpc_chttp2_stream* s) {
242-
auto* call_attempt_tracer =
243-
s->arena->GetContext<grpc_core::CallTracerInterface>();
244-
if (call_attempt_tracer == nullptr || !call_attempt_tracer->IsSampled()) {
245-
return nullptr;
246-
}
247-
return call_attempt_tracer->StartNewTcpTrace();
248-
}
249-
250239
grpc_core::WriteTimestampsCallback g_write_timestamps_callback = nullptr;
251240
grpc_core::CopyContextFn g_get_copied_context_fn = nullptr;
252241
} // namespace
@@ -1652,7 +1641,6 @@ static void perform_stream_op_locked(void* stream_op,
16521641
if (s->t->is_client && op->send_initial_metadata) {
16531642
s->call_tracer = s->arena->GetContext<grpc_core::CallTracerInterface>();
16541643
}
1655-
s->tcp_tracer = TcpTracerIfSampled(s);
16561644
if (GRPC_TRACE_FLAG_ENABLED(http)) {
16571645
LOG(INFO) << "perform_stream_op_locked[s=" << s << "; op=" << op
16581646
<< "]: " << grpc_transport_stream_op_batch_string(op, false)

src/core/ext/transport/chttp2/transport/internal.h

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,6 @@
7373
#include "src/core/lib/transport/transport.h"
7474
#include "src/core/lib/transport/transport_framing_endpoint_extension.h"
7575
#include "src/core/telemetry/call_tracer.h"
76-
#include "src/core/telemetry/tcp_tracer.h"
7776
#include "src/core/util/bitset.h"
7877
#include "src/core/util/debug_location.h"
7978
#include "src/core/util/ref_counted.h"
@@ -677,9 +676,6 @@ struct grpc_chttp2_stream {
677676
// TODO(yashykt): Remove this once call_tracer_transport_fix is rolled out
678677
grpc_core::CallTracerAnnotationInterface* parent_call_tracer = nullptr;
679678

680-
/// Only set when enabled.
681-
std::shared_ptr<grpc_core::TcpTracerInterface> tcp_tracer;
682-
683679
// time this stream was created
684680
gpr_timespec creation_time = gpr_now(GPR_CLOCK_MONOTONIC);
685681

src/core/ext/transport/chttp2/transport/writing.cc

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -699,10 +699,9 @@ grpc_chttp2_begin_write_result grpc_chttp2_begin_write(
699699
grpc_core::GrpcHttp2GetCopyContextFn();
700700
if (copy_context_fn != nullptr &&
701701
grpc_core::GrpcHttp2GetWriteTimestampsCallback() != nullptr) {
702-
t->context_list->emplace_back(copy_context_fn(s->arena),
703-
outbuf_relative_start_pos,
704-
num_stream_bytes, s->byte_counter,
705-
s->write_counter - 1, s->tcp_tracer);
702+
t->context_list->emplace_back(
703+
copy_context_fn(s->arena), outbuf_relative_start_pos,
704+
num_stream_bytes, s->byte_counter, s->write_counter - 1, nullptr);
706705
}
707706
}
708707
outbuf_relative_start_pos += num_stream_bytes;

0 commit comments

Comments
 (0)