Async client memory leak

[bale-john]

Hi guys,

I hope this message finds you well. I encountered a memory leak issue while using HeaderClientChannel in my code. The version I'm using is 2021.03.08.00.

Here's my usage pattern:

template <class AsyncClient>
std::shared_ptr<AsyncClient> RpcClientMan<AsyncClient>::CreateClient(
    folly::EventBase* evb, folly::SocketAddress addr, uint32_t timeout_ms,
    int32_t retry) {
  std::shared_ptr<apache::thrift::RequestChannel> channel =
      apache::thrift::ReconnectingRequestChannel::newChannel(
          *evb,
          [protocol_type = opts_.protocol_type, protocol_id = opts_.protocol_id,
           addr, timeout_ms](folly::EventBase& eb) mutable {
            std::shared_ptr<folly::AsyncSocket> socket;
            eb.runImmediatelyOrRunInEventBaseThreadAndWait(
                [&socket, &eb, addr, timeout_ms]() {
                  socket = folly::AsyncSocket::newSocket(&eb, addr, timeout_ms);
                });
            auto headerClientChannel =
                apache::thrift::HeaderClientChannel::newChannel(
                    std::move(socket));
            if (timeout_ms > 0) {
              headerClientChannel->setTimeout(timeout_ms);
            }

            headerClientChannel->setClientType(protocol_type);
            headerClientChannel->setProtocolId(protocol_id);

            return headerClientChannel;
          });

  channel->getProtocolId();

  std::shared_ptr<AsyncClient> client(
      new AsyncClient(apache::thrift::RetryingRequestChannel::newChannel(
          *evb, retry, std::move(channel))),
      [evb](auto* p) {
        evb->runImmediatelyOrRunInEventBaseThreadAndWait([p] { delete p; });
      });

  return client;
}

In my unit tests, I noticed that the HeaderClientChannel object is not being destructed properly, indicating a memory leak. Additionally, while troubleshooting this memory leak, I discovered another issue.

In the ReconnectingRequestChannel class, there's a method called reconnectIfNeeded():

void ReconnectingRequestChannel::reconnectIfNeeded() {
  if (!impl_ || !impl_->good()) {
    impl_ = implCreator_(evb_);
  }
}

This method can be accessed by multiple threads without any locking mechanism. Because client will be used by multi thread, when they send a message for the first time, they will invoke this method. Could this potentially lead to thread safety issues?

Thank you for your assistance!