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 <[email protected]> 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 <[email protected]> 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 < >> [email protected]> 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 <[email protected]> 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 <[email protected]> >>>> 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 <[email protected]> >>>>> 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 [email protected]. >>>>>> 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 >>> [email protected]. >>> 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 [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/capnproto/CAF8PYMhZZVfoow8bZHC1ERjGLU6PdvWeZ3%3DFbP2ooKR5hi7e7A%40mail.gmail.com.
