5

I'm getting requests through a (thread-safe) queue. Each request than needs to be handled in a separate thread. There is a chance that the function (which is actually calling a Java-program via _popen and polling its output) takes a very long time. From the main thread I need a mechanism to indicate that situation (basically, measuring thread-running-time).

In my example I'm trying to 'enrich' std::future with some time information. The sample runs seamlessly, but I'm uncertain if this is the correct way. Also, even if it is the 'correct' way, I'm having trouble writing a copy-constructor and a assignment operator for CFutureTest, which I would like to control myself.

Here is a very simple demo that mimics what I'm trying to achieve:

typedef std::future<int> FutureResultInt;

int ThreadFunc() {
  std::random_device rd;
  std::mt19937 mt(rd());
  const int iRand = std::uniform_int_distribution<int>(2000, 6000)(mt);
  std::cout << "ThreadFunc waiting for [" << iRand << "] ms ... " << std::endl;
  std::this_thread::sleep_for(std::chrono::milliseconds(iRand));
  std::cout << "ThreadFunc [" << iRand << "] done" << std::endl;
  return iRand;
}

class CFutureTest {
 public:
  CFutureTest() = delete;
  CFutureTest(FutureResultInt&& fr)
      : m_start(std::chrono::system_clock::now()), m_result() {
    m_result = std::move(fr);
  };

  int GetAge() const {
    return std::chrono::duration_cast<std::chrono::milliseconds>(
               std::chrono::system_clock::now() - m_start)
        .count();
  }

  // private:
  FutureResultInt m_result;
  std::chrono::time_point<std::chrono::system_clock> m_start;
};

int main() {
  std::vector<CFutureTest> futures;
  for (int i = 0; i < 5; i++)
    futures.push_back(std::move(std::async(std::launch::async, ThreadFunc)));
  while (futures.size() > 0) {
    for (std::vector<CFutureTest>::iterator it = futures.begin();
         it != futures.end(); ++it) {
      CFutureTest& future = *it;
      const std::future_status stat =
          future.m_result.wait_for(std::chrono::milliseconds(1));
      switch (stat) {
        case std::future_status::timeout:
          if (future.GetAge() > 4000) {
            std::cout << "Thread has exceeded the time limit" << std::endl;
          }
          continue;
        case std::future_status::deferred:
          std::cout << "std::future_status::deferred" << std::endl;
          continue;
      }
      const int iResult = future.m_result.get();
      std::cout << "future returned [" << iResult << "] (removing!)"
                << std::endl;
      futures.erase(it);
      if (futures.size() < 1) break;
      it = futures.begin();
    }
  }
  return 0;
}

rsjaffe
  • 5,600
  • 7
  • 27
  • 39
E.F.
  • 61
  • 2
  • A sidenote—you don't need to move from `std::async` return value, since it is an rvalue already. You can also initialize `m_result` member variable directly in constructor initializer list by `m_result(std::move(fr))`, which would generally be more efficient. Anyway, copy constructor/assignment for `CFutureTest` makes no sense, since the `m_result` member variable is not copyable. – Daniel Langr Sep 17 '18 at 05:40
  • Thanks Daniel for your comment. How does vector internally handles `CFutureTest` if copy ctor/assignment oper is not available? Plus, if I add `CFutureTest(const CFutureTest& src ) = delete;` the compiler claims `attempting to reference a deleted function` (which kinda proofs that the copy-ctor is needed and used). – E.F. Sep 17 '18 at 06:22
  • For `push_back`ing rvalues into `std::vector`, you just need a move constructor, which is in some cases generated by a compiler automatically for you. Such in a case of your original code. However, if you `delete` copy constructor, then you effectively prevent such generation of move constructor (see [this question](https://stackoverflow.com/q/37276413/580083) for details). You can then generate move constructor by `CFutureTest(CFutureTest&&) = default;`. – Daniel Langr Sep 17 '18 at 06:35
  • Thanks for the clarification. Besides your first note ('you don't need to move from std::async return value, since it is an rvalue already') - would you say that the general thought in my example is 'ok'? – E.F. Sep 17 '18 at 06:51
  • 1
    Now, I'm afraid that I don't have time to investigate your whole code in details. However, note that https://codereview.stackexchange.com/ might be a more appropriate site for code reviews. – Daniel Langr Sep 17 '18 at 07:31
  • I'll post the code there, thanks for the hint. And thank you for your time!! – E.F. Sep 17 '18 at 07:44
  • I'm voting to close this question as off-topic because it belongs on codereview.stackexchange.com – Mikhail Sep 21 '18 at 05:56
  • @Mikhail Except this is an example demo looking for validation, not real code looking for a review. – Mast Sep 21 '18 at 06:09
  • @Mast I don't understand what kind of response the author could expect except for stylistic review, or functionality checking. The functionality is fine, as he already notes. So what exactly is the question? – Mikhail Sep 21 '18 at 06:12
  • [Cross-posted on Code Review](https://codereview.stackexchange.com/q/203856/52915) – Mast Sep 21 '18 at 07:35

0 Answers0