Timeout thrower using multithreading

Question

I designed a simple timeout thrower for a bluetooth protocol I am writing. If the packet is not received in a certain amount of time, then timeout is thrown. I made it to be as plug and play as possible.

timeout.hpp:

#ifndef __timeout_h__
#define __timeout_h__

#include <thread>

struct Timeoutthread{
    private:
        int sleep;
    public:
        Timeoutthread(int seconds);
        void time(bool* alert);
};

class Timeout{
    private:
        int sleep;
        bool alert;
    public:
        Timeout(int seconds);
        bool timeout();
};

#endif

timeout.cpp:

#include "timeout.hpp"

Timeout::Timeout(int seconds){
    sleep = seconds;
    Timeoutthread timeoutthread(sleep);
    std::thread timeout(&Timeoutthread::time,timeoutthread,&alert);
    timeout.detach();
}

bool Timeout::timeout(){
    return alert;
}

Timeoutthread::Timeoutthread(int seconds){
    sleep = seconds;
}

void Timeoutthread::time(bool* alert){
    std::this_thread::sleep_for (std::chrono::seconds(sleep));
    *alert = true;
}

simple implementation:

#include <iostream>
#include "timeout.hpp"

int main(){
     std::string message = "";
     Timeout timeout(10);
     while(message == ""){
          message = readBLE();
          if(timeout.timeout() == true)
               message = "timed out";
     }

     std::cout<<message<<std::endl;

     return 0;
}

The loop waits for a bluetooth message, but if no message is received in 10 seconds, then a timeout is thrown.

Is this an efficient way to do it. I want to make sure that I am not wasting any resources.

EDIT:

There seems to be some confusion on the readBLE() part. There is a lot more code associated with this part but it does not act like std::cin where it is waiting for a user input. Whenever readBLE() is called, if there was no message received, then it would just return an empty string. Sorry for the confusion.

Answer 1

The Timeout class doesn't solve the problem you have

If you write:

message = (read from bluetooth);
if(timeout.timeout() == true)
    message = "timed out";

Then it will first wait for the message to be read from Bluetooth, which might take more than 10 seconds, and then once you have the message, it will check whether more than 10 seconds have passed since the start, and if so it will discard the message you got. The fact that the timer is run in its own thread does not magically make (read from bluetooth) exit after the timer expires.

What you instead have to do is run the (read from bluetooth) command in a thread, and wait at most 10 seconds for that to complete. With C++11, you can do this very easily with std::async():

#include <future>
#include <chrono>

...

auto future = std::async(std::launch::async, [] {
    return (read from bluetooth);
});

auto status = future.wait_for(std::chrono::seconds(10));

if (status == std::future_status::ready)
    message = future.get();
else
    message = "timed out";

The problem however is that if there is a timeout, the thread running the Bluetooth read command is still running. When exiting the scope, the destructor of future will block until the thread finishes execution. So this kind of approach has a limited use.

The best solution would be to find some wait to make (read from bluetooth) itself give up after 10 seconds, or have some way to cause it to stop waiting for data.

Identifiers with double underscores are reserved

You should not use identifiers that start with underscores, or contain double underscores, as they are reserved, and might be used by the compiler and/or the standard library. This even applies to macros, so instead of:

#ifndef __timeout_h__
#define __timeout_h__

Write:

#ifndef timeout_h
#define timeout_h

Or use the following pragma that most compilers understand, and that ensure that a header file is only read once:

#pragma once

Store durations as std::chrono::duration

Avoid storing timeouts as int, this limits the resolution. Instead, consider using std::chrono::duration to store the timeout period.

Make member functions const where appropriate

If a member function doesn't modify any member variables, mark it as const, like so:

class Timeout {
    ...
    bool timeout() const;
}

Ensure variables are properly initialized

You never initialize alert to false, so a call to timeout() might return an uninitialized value.

Use std::atomic<> variables when communicating between threads

If you write:

Timeout timeout(10);
// do something
if (timeout.timeout())
    ...

Then the compiler might know that alert is set to false in the first line, and if it can prove that do something never touches the variable timeout, then it can assume in the third line that alert will always be false. To ensure the compiler does not make such assumptions when threads are involved, you have to tell it that it should atomically read and write this flag.

Do you need a separate thread at all?

The only thing your Timeoutthread does is sleep for a certain amount of time, then set a variable. You know that the threads sets that variable a given number of seconds after it starts. So instead of using a thread, you can just store the current time when an instance of Timeout is created in a separate member variable, and in timeout() just check the difference between the current time now and the time stored in that member variable.