Sounds good. Relaxed is what I went with and it seemed to make TSAN happy. I uploaded a pull request for this (and the UBSAN issue).
Thanks for your guidance on this. On Sat, Oct 10, 2020 at 12:14 PM Kenton Varda <ken...@cloudflare.com> wrote: > An atomic read with "relaxed" ordering should be free. Even "acquire" > ordering (if TSAN insists on it) is free on x86. So I'd just have it always > be atomic. > > -Kenton > > On Fri, Oct 9, 2020 at 4:23 PM Vitali Lovich <vlov...@gmail.com> wrote: > >> Ok. That seems to have made TSAN happy (I still don't understand the >> exact reason it's safe but I'll trust you're higher level reasoning about >> the internals of capnp :D). >> >> Should this atomic read happen only when running under TSAN or just bite >> the bullet & do it regardless? It's unclear to me if this is a hotpath >> that's carefully tuned >> >> On Fri, Oct 9, 2020 at 10:00 AM 'Kenton Varda' via Cap'n Proto < >> capnproto@googlegroups.com> wrote: >> >>> It looks like the writes are done atomically, but the reads aren't. TSAN >>> is right to be suspicious of this. In practice there is no bug here, but >>> showing that requires higher-level reasoning that TSAN wouldn't be expected >>> to figure out. >>> >>> -Kenton >>> >>> On Fri, Oct 9, 2020 at 11:52 AM Vitali Lovich <vlov...@gmail.com> wrote: >>> >>>> So you're saying just change the read to atomic read? >>>> >>>> Is it possible there's a legitimate read-before-write race condition & >>>> that's what TSAN is complaining about? It doesn't seem to be complaining >>>> about concurrent writes but I also haven't investigated this codepath to >>>> understand yet as I assumed the problem was in my code. >>>> >>>> On Fri, Oct 9, 2020 at 9:48 AM Kenton Varda <ken...@cloudflare.com> >>>> wrote: >>>> >>>>> I think that ThreadSanitizer is having trouble recognizing that the >>>>> initialization of `brokenCapFactory` is thread-safe, due to the awkward >>>>> way >>>>> in which it is initialized. It may end up being initialized by multiple >>>>> threads, but all threads will initialize it to the same value, hence no >>>>> atomics are necessary when reading it later. >>>>> >>>>> Maybe we should use atomic reads anyway, to make ThreadSanitizer >>>>> happy. Doing so should be free, at least on x86. >>>>> >>>>> (The reason for the weird initialization is that the factory is >>>>> implemented in libcapnp-rpc.so, yet needs to be callable from libcapnp.so >>>>> -- but only if RPC is in use.) >>>>> >>>>> -Kenton >>>>> >>>>> On Fri, Oct 9, 2020 at 8:37 AM Vitali Lovich <vlov...@gmail.com> >>>>> wrote: >>>>> >>>>>> I'm trying to do a basic in-process connection of the servers & >>>>>> running that under TSAN but I feel like this is some nuance of the APIs >>>>>> I'm >>>>>> missing or using the API totally incorrectly outright, so I would >>>>>> appreciate some feedback if possible. I'm sure the bug must be in my code >>>>>> rather than cap'n'proto. I don't have a helpful standalone piece of code >>>>>> to >>>>>> demonstrate this issue at the moment (but if it's really critical I can >>>>>> work on providing that). I've provided brief snippets of what the threads >>>>>> do (the threads themselves are really that simple, it's just the schema & >>>>>> the threads using various helper internal libraries that make it harder >>>>>> to >>>>>> post a working standalone example). >>>>>> >>>>>> Full TSAN (to avoid spamming the list): https://pastebin.com/hSkVDgsU >>>>>> >>>>>> For context, the main thread does >>>>>> auto ioContext = kj::setupAsyncIo(); >>>>>> auto serverThread = ioContext.provider->newPipeThread(...); >>>>>> auto serverPtr = <wait for CV state from T1>; >>>>>> capnp::TwoPartyClient rpcClient(*serverThread.pipe); >>>>>> auto client = rpcClient.bootstrap().castAs<MyServer>(); >>>>>> auto connectedResponse = >>>>>> client.connectRequest().send().wait(ioContext.waitScope); *<-- this >>>>>> is the problematic TSAN line * >>>>>> >>>>>> T1 (taking AsyncIoProvider&, AsyncIoStream& stream, kj::WaitScope& >>>>>> waitScope): >>>>>> capnp::TwoPartyVatNetwork network(stream, >>>>>> capnp::rpc::twoparty::Side::SERVER); >>>>>> auto myServer = kj::heap<MyServerImpl>(); >>>>>> auto myServerPtr = myServer.get() >>>>>> auto rpcServer = capnp::makeRpcServer(network, kj::mv( myServer)); >>>>>> <publish myServerPtr to main thread using CV> >>>>>> network.onDisconnect().wait(waitScope);* <- problematic TSAN line* >>>>>> >>>>>> SUMMARY: ThreadSanitizer: data race >>>>>> capnproto/c++/src/capnp/layout.c++:2188:5 in >>>>>> capnp::_::WireHelpers::readCapabilityPointer(capnp::_::SegmentReader*, >>>>>> capnp::_::CapTableReader*, capnp::_::WirePointer const*, int) >>>>>> >>>>>> Read of size 8 at 0x0000016673e0 by main thread: >>>>>> #0 >>>>>> capnp::_::WireHelpers::readCapabilityPointer(capnp::_::SegmentReader*, >>>>>> capnp::_::CapTableReader*, capnp::_::WirePointer const*, int) >>>>>> capnproto/c++/src/capnp/layout.c++:2188:5 (ld-2.26.so+0x809e60) >>>>>> #1 capnp::_::PointerReader::getCapability() const >>>>>> capnproto/c++/src/capnp/layout.c++:2705 (ld-2.26.so+0x809e60) >>>>>> #2 >>>>>> capnp::AnyPointer::Reader::getPipelinedCap(kj::ArrayPtr<capnp::PipelineOp >>>>>> const>) const capnproto/c++/src/capnp/any.c++:53:18 (ld-2.26.so >>>>>> +0x7ff4a7) >>>>>> #3 capnp::_::(anonymous >>>>>> >>>>>> Previous atomic write of size 8 at 0x0000016673e0 by thread T1: >>>>>> #0 __tsan_atomic64_store <null> (ld-2.26.so+0x6911ee) >>>>>> #1 >>>>>> capnp::_::setGlobalBrokenCapFactoryForLayoutCpp(capnp::_::BrokenCapFactory&) >>>>>> capnproto/c++/src/capnp/layout.c++:45:3 (ld-2.26.so+0x800d12) >>>>>> #2 >>>>>> capnp::ReaderCapabilityTable::ReaderCapabilityTable(kj::Array<kj::Maybe<kj::Own<capnp::ClientHook> >>>>>> > >) capnproto/c++/src/capnp/capability.c++:951:3 (ld-2.26.so >>>>>> +0x6f32cc) >>>>>> #3 capnp::_::(anonymous >>>>>> namespace)::RpcConnectionState::RpcCallContext::RpcCallContext(capnp::_::(anonymous >>>>>> namespace)::RpcConnectionState&, unsigned int, >>>>>> kj::Own<capnp::IncomingRpcMessage>&&, >>>>>> kj::Array<kj::Maybe<kj::Own<capnp::ClientHook> > >, >>>>>> capnp::AnyPointer::Reader const&, bool, >>>>>> kj::Own<kj::PromiseFulfiller<void> >>>>>> >&&, unsigned long, unsigned short) >>>>>> >capnproto/c++/src/capnp/rpc.c++:2144:11 >>>>>> (ld-2.26.so+0x754bea) >>>>>> >>>>>> -- >>>>>> You received this message because you are subscribed to the Google >>>>>> Groups "Cap'n Proto" group. >>>>>> To unsubscribe from this group and stop receiving emails from it, >>>>>> send an email to capnproto+unsubscr...@googlegroups.com. >>>>>> To view this discussion on the web visit >>>>>> https://groups.google.com/d/msgid/capnproto/19bac31b-6f11-43d1-886e-93d6bc32557an%40googlegroups.com >>>>>> <https://groups.google.com/d/msgid/capnproto/19bac31b-6f11-43d1-886e-93d6bc32557an%40googlegroups.com?utm_medium=email&utm_source=footer> >>>>>> . >>>>>> >>>>> -- >>> You received this message because you are subscribed to a topic in the >>> Google Groups "Cap'n Proto" group. >>> To unsubscribe from this topic, visit >>> https://groups.google.com/d/topic/capnproto/634juhn5ap0/unsubscribe. >>> To unsubscribe from this group and all its topics, send an email to >>> capnproto+unsubscr...@googlegroups.com. >>> To view this discussion on the web visit >>> https://groups.google.com/d/msgid/capnproto/CAJouXQkT-3dHBk3iqE_fsfngaFZ447yJK9APof1VEKh-9RTHVw%40mail.gmail.com >>> <https://groups.google.com/d/msgid/capnproto/CAJouXQkT-3dHBk3iqE_fsfngaFZ447yJK9APof1VEKh-9RTHVw%40mail.gmail.com?utm_medium=email&utm_source=footer> >>> . >>> >> -- You received this message because you are subscribed to the Google Groups "Cap'n Proto" group. To unsubscribe from this group and stop receiving emails from it, send an email to capnproto+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/capnproto/CAF8PYMhZZVfoow8bZHC1ERjGLU6PdvWeZ3%3DFbP2ooKR5hi7e7A%40mail.gmail.com.
