Discussion :

[Guillaume1701]

Bonjour à tous,

J’essaye d’implémenter un « one shot timer » en c++11. Après divers recherche, je suis arrivé au code suivant :

timer.h

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
#ifndef TIMER_H
#define TIMER_H
 
#include <thread>
#include <functional>
#include <atomic>
#include <mutex>
#include <condition_variable>
 
class Timer {
	public:
		Timer() noexcept;
		~Timer() noexcept;
 
		void stop(void) noexcept;
		void start(int interval_ms, std::function<void(void)> func) noexcept;
		bool isRunning(void) const noexcept;
 
	private:
		std::atomic<bool> m_bExecute;
		std::thread m_thd;
		std::mutex m_oMutex;
		std::condition_variable m_oCondition;
};
 
#endif // TIMER_H

timer.cpp

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
#include "timer.h"
 
Timer::Timer() noexcept :
	m_bExecute(false)
{
}
 
Timer::~Timer() noexcept
{
	if (m_bExecute.load(std::memory_order_acquire))
	{
		stop();
	}
}
 
void Timer::stop(void) noexcept
{
	m_oCondition.notify_one();
	m_bExecute.store(false, std::memory_order_release);
	if (m_thd.joinable())
	{
		m_thd.join();
	}
}
 
void Timer::start(int interval_ms, std::function<void(void)> func) noexcept
{
	if (m_bExecute.load(std::memory_order_acquire))
	{
		stop();
	}
 
	m_bExecute.store(true, std::memory_order_release);
	m_thd = std::thread([this, interval_ms, func]()
	{
		std::unique_lock<std::mutex> lock(m_oMutex);
		if (m_oCondition.wait_for(lock, std::chrono::milliseconds(interval_ms)) == std::cv_status::timeout)
		{
			func();
		}
	});
}
 
bool Timer::isRunning(void) const noexcept
{
	return (m_bExecute.load(std::memory_order_acquire) && m_thd.joinable());
}

Pour tester ma classe, j’utilise le code suivant :

main.cpp

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
#include <iostream>
#include <cassert>
#include <thread>
#include <chrono>
 
#include "timer.h"
 
void test_class_timer_1();
void test_class_timer_2();
void test_class_timer_3();
void test_class_timer_4();
 
void callbackTimer(void);
bool bCallbackTimer;
 
int main()
{
    std::cout << std::endl << "Test timer" << std::endl << std::endl;
 
    std::cout << "----- test_class_timer_1 -----" << std::endl;
    test_class_timer_1();
    std::cout << "------------------------------" << std::endl;
 
    std::cout << "----- test_class_timer_2 -----" << std::endl;
    test_class_timer_2();
    std::cout << "------------------------------" << std::endl;
 
    std::cout << "----- test_class_timer_3 -----" << std::endl;
    test_class_timer_3();
    std::cout << "------------------------------" << std::endl;
 
    std::cout << "----- test_class_timer_4 -----" << std::endl;
    test_class_timer_4();
    std::cout << "------------------------------" << std::endl;
 
    std::cout << std::endl << "End test timer" << std::endl << std::endl;
 
    return EXIT_SUCCESS;
}
 
void test_class_timer_1()
{
    Timer oTimer;
 
    bCallbackTimer = false;
    oTimer.start(500, &callbackTimer);
    assert(oTimer.isRunning() == true);
    std::this_thread::sleep_for(std::chrono::milliseconds(1000));
    assert(bCallbackTimer == true);
}
 
void test_class_timer_2()
{
    Timer oTimer;
 
    bCallbackTimer = false;
    oTimer.start(3000, &callbackTimer);
    assert(oTimer.isRunning() == true);
    std::this_thread::sleep_for(std::chrono::milliseconds(500));
    assert(bCallbackTimer == false);
    oTimer.stop();
    assert(oTimer.isRunning() == false);
    assert(bCallbackTimer == false);
    std::this_thread::sleep_for(std::chrono::milliseconds(4000));
}
 
void test_class_timer_3()
{
    Timer oTimer;
 
    bCallbackTimer = false;
    oTimer.start(2000, &callbackTimer);
    std::this_thread::sleep_for(std::chrono::milliseconds(1000));
    oTimer.start(2000, &callbackTimer);
    std::this_thread::sleep_for(std::chrono::milliseconds(1000));
    oTimer.start(2000, &callbackTimer);
    std::this_thread::sleep_for(std::chrono::milliseconds(1000));
    assert(oTimer.isRunning() == true);
    assert(bCallbackTimer == false);
    std::this_thread::sleep_for(std::chrono::milliseconds(1500));
    assert(bCallbackTimer == true);
}
 
void test_class_timer_4()
{
    Timer oTimer;
 
    bCallbackTimer = false;
    oTimer.start(2000, &callbackTimer);
    oTimer.start(2000, &callbackTimer);
    oTimer.start(2000, &callbackTimer);
    oTimer.start(2000, &callbackTimer);
    assert(bCallbackTimer == false);    
    std::this_thread::sleep_for(std::chrono::milliseconds(3000));
    assert(bCallbackTimer == true);
}
 
void callbackTimer(void)
{
    std::cout << "callbackTimer" << std::endl;
    bCallbackTimer = true;
}

Et j’obtiens le résultat suivant :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
 
Test timer
 
----- test_class_timer_1 -----
callbackTimer
------------------------------
 
----- test_class_timer_2 -----
------------------------------
 
----- test_class_timer_3 -----
callbackTimer
------------------------------
 
----- test_class_timer_4 -----
callbackTimer
callbackTimer
callbackTimer
a.out: src/main.cpp:93: void test_class_timer_4(): Assertion 'bCallbackTimer == false' failed.
Abandon (core dumped)

Je remarque que j’ai surement un problème de concurrence mais je ne vois pas où et comment le corriger.

Avez-vous des remarques sur le code et / ou trouvé un problème dans ce dernier. De mon côté, je ne sais pas quoi modifier ou améliorer car je n’ai pas vraiment utilisé les variables conditionnelles auparavant.

Merci d’avance

[Bousk]

- Un constructeur sans paramètre n'a pas à être explicit
- Le destructeur virtuel est inutil ici
- oTimer.start(3000; &callbackTimer); ne compile pas

Il est possible que la création du thread, copie du foncteur etc prenne plus que les 2s de ton timeout. Ça me semble long même pour créer un thread, mais pourquoi pas...
Ça expliquerait pourquoi ta callback est appelée, alors que je suppose tu t'attends à ce qu'elle ne le soit pas.

Potentiel fix simple : réinitialiser execute après le join et ajouter un check de execute dans le if de la callback pour savoir si oui ou non elle doit être appelée ?
Tu es peut-être victime de Spurious wake-up.
https://stackoverflow.com/a/16350623

[Guillaume1701]

Il est possible que la création du thread, copie du foncteur etc prenne plus que les 2s de ton timeout. Ça me semble long même pour créer un thread, mais pourquoi pas...
Ça expliquerait pourquoi ta callback est appelée, alors que je suppose tu t'attends à ce qu'elle ne le soit pas.

Effectivement je ne souhaite pas que ma callback soit appelée. De même, je ne pense pas que la création d'un thread soit aussi long. C'est pour cela que dans l'autre test, j'ai mis une attente entre chaque appel à la fonction start().

[Bousk]

Un breakpoint dans la callback t'indiquera directement et très facilement d'où l'appel vient.

Edit de plus haut :
Potentiel fix simple : réinitialiser execute après le join et ajouter un check de execute dans le if de la callback pour savoir si oui ou non elle doit être appelée ?
Tu es peut-être victime de Spurious wake-up.
https://stackoverflow.com/a/16350623

[Guillaume1701]

Potentiel fix simple : réinitialiser execute après le join et ajouter un check de execute dans le if de la callback pour savoir si oui ou non elle doit être appelée ?

Ajouter une action après le join ne changera rien car il faut que la thread se termine et donc la callback sera déjà appelée. Mais je me trompe peut-être.

[Bousk]

Je me suis emmêlé, ce serait plutôt passer execute à false avant le notify. La partie de checker execute dans la callback reste valide et nécessaire.
Sinon tu peux aussi utiliser un compteur/identifiant d'appel au lieu d'un simple bool. Ça devrait permettre de faire de multiples Start et que seul le dernier soit vraiment exécuté. Tu oublies la condition variable pour un simple sleep thread et au lieu de join tu detach le thread quand tu n'en as plus besoin.

[Guillaume1701]

Je me suis emmêlé, ce serait plutôt passer execute à false avant le notify. La partie de checker execute dans la callback reste valide et nécessaire.

Effectivement cela corrige mon problème