Race condition between "send request" and "receive response" threads

[ltaurines]

Description

The issue is that the response thread notification might be sent (and so arrive) before the request thread is ready to wait for it. Then the request thread timeouts and rethrows the exception that calls (whithout the proposed fixed of issue #349: try-catch block added to KeepAliveThread::Run())) std::terminate and abort. It is also likely to be the explanation of issue #377 => KeepAliveThread Stopped!.

(file binary_client.cpp)

      Response Send(Request request) const {
            request.Header = CreateRequestHeader();

            RequestCallback<Response> requestCallback(Logger);
            ResponseCallback responseCallback = [&requestCallback](std::vector<char> buffer, ResponseHeader h) {
                  requestCallback.OnData(std::move(buffer), std::move(h));
            };
            std::unique_lock<std::mutex> lock(Mutex);
            Callbacks.insert(std::make_pair(request.Header.RequestHandle, responseCallback));
            lock.unlock();

            LOG_DEBUG(Logger, "binary_client         | send: id: {} handle: {}, UtcTime: {}", ToString(request.TypeId, true), request.Header.RequestHandle, request.Header.UtcTime);

            Send(request);

// ????????????????????????????????????
// Here, the OS sceduler may interrupt this thread and resume the response thread 
// that calls the following code (file binary_client.cpp):

void OnData(std::vector<char> data, ResponseHeader h) {
      Data = std::move(data);
      this->header = std::move(h);
      doneEvent.notify_all();             // KO => request thread MIGHT NOT BE YET WAITING for this notification
}
// ????????????????????????????????????

            Response res;
            try {
// ????????????????????????????????????
            // Only here, this thread is ready to deal with the response thread notification 
            // (more precisely when the fonction WaitForData calls doneEvent.wait_for(lock, msec);) 

                  res = requestCallback.WaitForData(std::chrono::milliseconds(request.Header.Timeout));
// ????????????????????????????????????
            }

            catch (std::exception& ex) {
                  //Remove the callback on timeout
                  std::unique_lock<std::mutex> lock(Mutex);
                  Callbacks.erase(request.Header.RequestHandle);
                  lock.unlock();
                  throw;
            }

            return res;
      }

How to reproduce

I isolated the issue with only the KeepAliveThread running which is the case just after connecting to an endpoint and sleeping until the KeepAliveThread terminate the process. In order to reproduce it faster I reduced the DefaultTimeout (divided by 480), hence the KeepAliveThread wakes up every 100ms approximately.
Note: We come across this issue quite often in production because our application (with hundred of threads) is running on VMs with only 2 threads of execution. So this yeilds a lot of context switches.

Fix proposal

In the response thread

void OnData(std::vector<char> data, ResponseHeader h) {
      Data = std::move(data);
      this->header = std::move(h);

????????????????????????????????????
      // The lock is only released by the function doneEvent.wait_for(lock, msec) uniquely 
      // called from the request thread. So when this reponse thread acquires and releases it,
      // we are 100% sûre that the request thread is ready to deal with the comming notification.
      std::unique_lock<std::mutex> lock(m);
      lock.unlock();
????????????????????????????????????

      doneEvent.notify_all();             // OK => request thread is ready to deal with this notification.
}

In the resquest thread (reduces lock contention)

Creating the response before sending the request will reduce the number of times the response thread will wait for the lock.

            Response res;
            Send(request);

Instead of

            Send(request);
            Response res;

[guojing555]

  您好，我最近正在休假中，无法亲自回复您的邮件。我将在假期结束后，尽快给您回复。