2

I'd like to use the TTheadedServer in a separate thread to have control on when to stop/start it. My application needs only 1 controlling thread and one processing thread. I don't expect to have more than one client as I'm using thrift as a relay. TSimpleServer is not thread-safe, so I dropped that option.

I made a little minimal example to check whether it's thread-safe, and used clang's thread-sanitizer to make sure it's thread-safe. Here's the example

std::shared_ptr<MyHandler> handler = std::make_shared<MyHandler>();

int port = 9090;

th::stdcxx::shared_ptr<th::TProcessor>         processor(new HandlerProcessor(handler));
th::stdcxx::shared_ptr<tht::TServerTransport>  serverTransport(new tht::TServerSocket(port));
th::stdcxx::shared_ptr<tht::TTransportFactory> transportFactory(
    new tht::TBufferedTransportFactory());
th::stdcxx::shared_ptr<thp::TProtocolFactory> protocolFactory(new thp::TBinaryProtocolFactory());

ths::TThreadedServer server(processor, serverTransport, transportFactory, protocolFactory);

// start in another thread
std::thread          t(&ths::TThreadedServer::serve, &server);
t.detach();

std::this_thread::sleep_for(std::chrono::seconds(5));

// stop in this thread
server.stop();

std::this_thread::sleep_for(std::chrono::seconds(5));

So what I simply do is start the server with serve() in another thread, then wait for some time, and stop it. I ran this with thread sanitizer, and got a few thread safety warnings. I mention 2 here:

First: thrift/lib/cpp/src/thrift/transport/TServerSocket.cpp:244, at:

interruptableChildren_ = enable;

Second: thrift/lib/cpp/src/thrift/transport/TServerSocket.cpp:654, at:

if (-1 == send(notifySocket, cast_sockopt(&byte), sizeof(int8_t), 0)) {
  GlobalOutput.perror("TServerSocket::notify() send() ", THRIFT_GET_SOCKET_ERROR);
}

So is what I'm doing correct? And is TThreadedServer controller thread-safe? Thread-sanitizer doesn't seem to think so, although the test program works with no problems.

I'm using Thrift 0.12.0.

The Quantum Physicist
  • 21,050
  • 13
  • 77
  • 143

1 Answers1

1

It's thread-safe, but it might have multi-threading bugs that will never manifest in practice. For example, in case of interruptableChildren_ - the flow is that you might configure its value in the main thread but then it's read by the acceptor thread (where TServerSocket::acceptImpl runs). In theory, you write and read from the unprotected variable. In practice, you never change it after you launch your server with std::thread t(&ths::TThreadedServer::serve, &server); line, so no data races will occur.

I am guessing notify() case is similar.

Roman
  • 1,105
  • 9
  • 23
  • Thank you for the answer. Can you provide some reference where this is mentioned? I can't find any. – The Quantum Physicist Dec 25 '18 at 09:37
  • I've not read their docs for a long time. 4 years ago I've written prod quality RPC service using thrift so I know based on my personal experience. I used much older version though (with more bugs, for sure :) ). However, here is short answer about various threading models in thrift: https://stackoverflow.com/a/28547917/2280111 – Roman Dec 25 '18 at 10:36
  • I know that post actually. It doesn't, however, answer the question I asked. It's quite weird that there's no docs that specify this. – The Quantum Physicist Dec 25 '18 at 11:29
  • I am not the maintainer of this package but I can try and explain the logic of this. You do not specify that TThreadedServer is thread-safe because it's like specifying that ships are resistant to water :) The whole goal of this component is to open multiple threads and manage them. "Thread" is even in its name. So they plan it to be thread-safe as long as you use it according to their expected flow. It might have bugs and it might be non-thread safe upon weird usage like calling a member function from a wrong thread or from a wrong phase. – Roman Dec 25 '18 at 12:05
  • Thanks for trying to explain it. While I agree with your conclusions, the assumptions may be incorrect. It may be possible to consider this usable in a blocking sense, where the user is supposed to block their main-thread, and if they're not happy with that, they may just use `TNonBlockingServer`. It's a huge problem that none of this is documented, because it opens the door to speculation. – The Quantum Physicist Dec 25 '18 at 12:14