Using threads; working, but would like input

[bedtime]

This program works fine, but I am new to C++ (dabbled 5yrs+ ago), and would like some input.

This program gathers data and then passes it off to 'xsetroot' to be displayed in my dashboard (dwm window manager). It gives CPU, Memory, Volume, Date/Time, and it uses threads to gather information in realtime. Email count, rss feed, and weather are gathered via execution from separate programs. Since they can take some time to gather info, and I want the program to start immediately, I thought this the perfect situation for threads. :)

I post as something tells me that I am not using the threads in the manner that they're intended to be used. This is my first attempt using them, and I would like to startoff on the right foot.

Most of the first functions can be safely ignored; the meat is in main() and just above. I am interested in anything I can do to more properly and efficiently use the 'thread' command.

Thanks.

// Compile with:
//
// g++ -std=c++11 -O2 -Wall bardemo.cpp -o bardemo -lasound -lpthread

#include<alsa/asoundlib.h>
#include<fstream>
#include<iostream>
#include<string>
#include<chrono>
#include<thread>
#include"sys/sysinfo.h"
#include <future>

using namespace std;

/* 	Functions which get information to display.
	Not essentially required for the question
	asked but here if they might prove useful. */

static unsigned long long lastTotalUser, lastTotalUserLow, lastTotalSys, lastTotalIdle;

void init(){
    FILE* file = fopen("/proc/stat", "r");
    fscanf(file, "cpu %llu %llu %llu %llu", &lastTotalUser, &lastTotalUserLow,
        &lastTotalSys, &lastTotalIdle);
    fclose(file);
}


std::string cpuPercent(){

	double percent;
	long int intPercent;
	std::string s;

	FILE* file;
	unsigned long long totalUser, totalUserLow, totalSys, totalIdle, total;

	file = fopen("/proc/stat", "r");
	fscanf(file, "cpu %llu %llu %llu %llu", &totalUser, &totalUserLow,
	&totalSys, &totalIdle);
	fclose(file);

	if (totalUser < lastTotalUser || totalUserLow < lastTotalUserLow ||
		totalSys < lastTotalSys || totalIdle < lastTotalIdle){
		//Overflow detection. Just skip this value.
		percent = -1.0;
	}else{
		total = (totalUser - lastTotalUser) + (totalUserLow - lastTotalUserLow) +
		(totalSys - lastTotalSys);
		percent = total * 100;
		total += (totalIdle - lastTotalIdle);
		percent /= total;

	}

	lastTotalUser = totalUser;
	lastTotalUserLow = totalUserLow;
	lastTotalSys = totalSys;
	lastTotalIdle = totalIdle;

	intPercent = percent;
	s = to_string(intPercent);

	// Stop overflow by making all numbers '100' if above 2 digits
	return (s.length() > 2 ? "100" : s);
}


std::string exec(const char* cmd) {

        std::array<char, 128> buffer;
        std::string result;
        std::shared_ptr<FILE> pipe(popen(cmd, "r"), pclose);
        if (!pipe) throw std::runtime_error("popen() failed!");

                while (!feof(pipe.get()))
                        if (fgets(buffer.data(), 128, pipe.get()) != nullptr){
                                result += buffer.data();
        }

        return result.substr(0, result.length() - 1);
}


std::string procs(void){

	struct sysinfo s;
        sysinfo(&s);

        return to_string(s.procs);
}


std::string date_and_time(void){

	static char date[48];
        time_t now = time(0);

	strftime(date, 48, "%a %b %d, %H:%M:%S", localtime(&now));

	std::string charToStr (date);

        return charToStr;
}


std::string get_vol(void)
{
	int vol;
	std::string volume;

	snd_hctl_t *hctl;
	snd_ctl_elem_id_t *id;
	snd_ctl_elem_value_t *control;

	// To find card and subdevice: /proc/asound/, aplay -L, amixer controls
	snd_hctl_open(&hctl, "hw:0", 0);
	snd_hctl_load(hctl);

	snd_ctl_elem_id_alloca(&id);
	snd_ctl_elem_id_set_interface(id, SND_CTL_ELEM_IFACE_MIXER);

	// amixer controls
	snd_ctl_elem_id_set_name(id, "Master Playback Volume");

	snd_hctl_elem_t *elem = snd_hctl_find_elem(hctl, id);

	snd_ctl_elem_value_alloca(&control);
	snd_ctl_elem_value_set_id(control, id);

	snd_hctl_elem_read(elem, control);
	snd_hctl_close(hctl);

	vol = (int)snd_ctl_elem_value_get_integer(control,0);
	volume = to_string(vol*100/127);

	return volume;
}


std::string get_mem_total() {

	std::string info[] = {"MemTotal:", "MemFree:", "Buffers:", "Cached:"};
        int intInfo[4];

	std::string token;
	std::ifstream file("/proc/meminfo");

	while(file >> token)
	{
		for(int i = 0; i < 4; i++)
			if(token == info[i]){
				file >> intInfo[i];

				if(i == 3)
					return std::to_string((intInfo[0] - intInfo[1] - (intInfo[2] + intInfo[3])) / 1024);
			}

	}

	return "Unable to grab info.";
}


void execFunc(const char * e, std::string &s, bool &done)
{
        s = exec(e);
        done = true;
}


int main(int argc, char* argv[])
{

	init();	// Used for CPU func.

	int  mSec	= 500;		// How long between display updates?
	bool tStarted	= false;	// Has the thread been started?
	int  sToUpdate	= 60;		// Seconds until threads update

	if (argc > 1) mSec = atoi(argv[1]);	// Take the value if given

	// Commands which user would like to run as threads
	static const char* const command[] = { "rssSub", "mailcheck", "weather" };

	// Calculate num. of threads
	size_t nThreads = (sizeof command  / sizeof command[0]);

	// Get thread stuff ready
	std::thread	myThreads[nThreads];
	std::string	funcStr[nThreads];		// The original string from the thread
	std::string	readyStr[nThreads];		// The string which will take a copy of funcStr which it completes
        bool 		tReady[nThreads];		// A means of indicating the thread has ran and is OK to copy strings

	// Prep timer which sleeps between checks
	std::chrono::milliseconds timespan(mSec);

	// To hold the final execution command
	std::string sysStrCmd;				// Gathers the command with easy to work with string, and...
        const char * sysCharCmd;			// ...gives it to system(), which takes a char const

	// Timers required for functions to update at proper intervals
	auto start_time		= std::chrono::high_resolution_clock::now();
	auto current_time	= std::chrono::high_resolution_clock::now();
	auto timeSince		= std::chrono::duration_cast<std::chrono::seconds>(current_time - start_time).count();

	while(1)	// Loop while the definition of '1' remains true
	{

		current_time = std::chrono::high_resolution_clock::now();
		timeSince = std::chrono::duration_cast<std::chrono::seconds>(current_time - start_time).count();

		// Do the threads need to be updated?
		if (!tStarted) {

			for (unsigned int i = 0; i < nThreads; i++)
			{
				myThreads[i] = std::thread(&execFunc, ref(command[i]), std::ref(funcStr[i]), ref(tReady[i]));
				myThreads[i].detach();
			}

			tStarted = true;

		}else if (timeSince >= sToUpdate) {

			// Reset clock as we've met our time to update
			start_time = std::chrono::high_resolution_clock::now();
			tStarted = 0;
		}

		// Keep checking to see if the threads are finished and update strings in realtime
		for (unsigned int i = 0; i < nThreads; i++)
			if (tReady[i]){				// Is the thread is done?
				tReady[i] = 0;			// Reset for now
				readyStr[i] = funcStr[i];	// Finally, the string is copied
			}

		std::string sysStrCmd = "xsetroot -name \"C:" +
		cpuPercent()	+ "% " +
		get_mem_total()	+ "m " +
		"P:" + procs()	+ " " +
		"V:" + get_vol()+ "% " +
		readyStr[0] + (readyStr[0] == "" ? "": " ") +
		readyStr[1] + (readyStr[1] == "" ? "": " ") +
		readyStr[2] + (readyStr[2] == "" ? "": " ") +
		date_and_time() + "\"";

		// system() takes a const char
		sysCharCmd = sysStrCmd.c_str();
		system(sysCharCmd);

		// Sleep in an efficient manner
		std::this_thread::sleep_for(timespan);
	}

	return 0;
}

[kbw]

One way of doing this is:

while (!stop)
    start agents to collect metrics
    wait for them all to return
    publish metrics
    sleep delay
endwhile

However, your agents are detached and run completely asynchronously. Is this what you really want?

[bedtime]

Yes, I want them to return on their own time. The idea is that the information from each of the threads is returned as soon as it finishes and does not have to wait for the others (which could be held up by bad connection and such).

[Cubbi]

There is still no need to call thread::detach (ever), but it's not the most important issue here: the important issue that jumps out at me is the data races on the array of non-atomic bools tReady[] and on the array of strings funcStr[]. One thread is writing and another thread is reading elements of both arrays, with no synchronization, that's undefined behavior.

Look into inter-thread communication: condition variables, atomic variables, maybe barriers or futures