Skip to content

Commit 3f67bf6

Browse files
metsmametsma
committed
Cleanup thread cleaning and fix PinPAD crash
Fixes #346 Signed-off-by: Raul Metsma <[email protected]>
1 parent a717afc commit 3f67bf6

File tree

7 files changed

+66
-135
lines changed

7 files changed

+66
-135
lines changed

‎src/controller/controller.cpp‎

Lines changed: 34 additions & 86 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
#include "threads/waitforcardthread.hpp"
2929

3030
#include "utils/utils.hpp"
31+
3132
#include "inputoutputmode.hpp"
3233
#include "writeresponse.hpp"
3334

@@ -44,19 +45,11 @@ const QString RESP_USER_CANCEL = QStringLiteral("ERR_WEBEID_USER_CANCELLED");
4445

4546
QVariantMap makeErrorObject(const QString& errorCode, const QString& errorMessage)
4647
{
47-
const auto errorBody = QVariantMap {
48-
{QStringLiteral("code"), errorCode},
49-
{QStringLiteral("message"), errorMessage},
50-
};
51-
return {{QStringLiteral("error"), errorBody}};
52-
}
53-
54-
void interruptThread(QThread* thread)
55-
{
56-
qDebug() << "Interrupting thread" << uintptr_t(thread);
57-
thread->disconnect();
58-
thread->requestInterruption();
59-
ControllerChildThread::waitForControllerNotify.wakeAll();
48+
return {{QStringLiteral("error"),
49+
QVariantMap {
50+
{QStringLiteral("code"), errorCode},
51+
{QStringLiteral("message"), errorMessage},
52+
}}};
6053
}
6154

6255
} // namespace
@@ -111,11 +104,14 @@ void Controller::startCommandExecution()
111104
REQUIRE_NON_NULL(commandHandler)
112105

113106
// Reader monitor thread setup.
114-
WaitForCardThread* waitForCardThread = new WaitForCardThread(this);
107+
auto* waitForCardThread = new WaitForCardThread(this);
108+
connect(this, &Controller::stopCardEventMonitorThread, waitForCardThread,
109+
&WaitForCardThread::requestInterruption);
115110
connect(waitForCardThread, &WaitForCardThread::statusUpdate, this, &Controller::statusUpdate);
116111
connect(waitForCardThread, &WaitForCardThread::cardsAvailable, this,
117112
&Controller::onCardsAvailable);
118-
saveChildThreadPtrAndConnectFailureFinish(waitForCardThread);
113+
connect(waitForCardThread, &ControllerChildThread::failure, this,
114+
&Controller::onCriticalFailure);
119115

120116
// UI setup.
121117
window = WebEidUI::createAndShowDialog(commandHandler->commandType());
@@ -126,33 +122,6 @@ void Controller::startCommandExecution()
126122
waitForCardThread->start();
127123
}
128124

129-
void Controller::saveChildThreadPtrAndConnectFailureFinish(ControllerChildThread* childThread)
130-
{
131-
REQUIRE_NON_NULL(childThread)
132-
// Save the thread pointer in child thread tracking map to request interruption and wait for
133-
// it to quit in waitForChildThreads().
134-
childThreads[uintptr_t(childThread)] = childThread;
135-
136-
connect(childThread, &ControllerChildThread::failure, this, &Controller::onCriticalFailure);
137-
138-
// When the thread is finished, remove the pointer from the tracking map and call deleteLater()
139-
// on it to free the thread object. Although the thread objects are freed through the Qt object
140-
// tree ownership system anyway, it is better to delete them immediately when they finish.
141-
connect(childThread, &ControllerChildThread::finished, this, [this, childThread]() {
142-
QScopedPointer<ControllerChildThread, QScopedPointerDeleteLater> deleteLater {childThread};
143-
144-
const auto threadPtrAddress = uintptr_t(childThread);
145-
if (childThreads.count(threadPtrAddress) && childThreads[threadPtrAddress]) {
146-
childThreads[threadPtrAddress] = nullptr;
147-
childThread->wait();
148-
qDebug() << "Thread" << threadPtrAddress << "finished";
149-
} else {
150-
qWarning() << "Controller child thread" << childThread
151-
<< "is missing or null in finish slot";
152-
}
153-
});
154-
}
155-
156125
void Controller::connectOkCancelWaitingForPinPad()
157126
{
158127
REQUIRE_NON_NULL(window)
@@ -192,9 +161,10 @@ void Controller::onCardsAvailable(
192161
void Controller::runCommandHandler(const std::vector<ElectronicID::ptr>& availableEids)
193162
{
194163
try {
195-
CommandHandlerRunThread* commandHandlerRunThread =
164+
auto* commandHandlerRunThread =
196165
new CommandHandlerRunThread(this, *commandHandler, availableEids);
197-
saveChildThreadPtrAndConnectFailureFinish(commandHandlerRunThread);
166+
connect(commandHandlerRunThread, &ControllerChildThread::failure, this,
167+
&Controller::onCriticalFailure);
198168
connectRetry(commandHandlerRunThread);
199169

200170
// When the command handler run thread retrieves certificates successfully, call
@@ -213,51 +183,32 @@ void Controller::runCommandHandler(const std::vector<ElectronicID::ptr>& availab
213183

214184
void Controller::onCertificatesLoaded()
215185
{
216-
CardEventMonitorThread* cardEventMonitorThread =
217-
new CardEventMonitorThread(this, std::string(commandType()));
218-
saveChildThreadPtrAndConnectFailureFinish(cardEventMonitorThread);
219-
cardEventMonitorThreadKey = uintptr_t(cardEventMonitorThread);
186+
auto* cardEventMonitorThread = new CardEventMonitorThread(this, commandType());
187+
connect(cardEventMonitorThread, &ControllerChildThread::failure, this,
188+
&Controller::onCriticalFailure);
189+
connect(this, &Controller::stopCardEventMonitorThread, cardEventMonitorThread,
190+
&WaitForCardThread::requestInterruption);
220191
connect(cardEventMonitorThread, &CardEventMonitorThread::cardEvent, this, &Controller::onRetry);
221192
cardEventMonitorThread->start();
222193
}
223194

224-
void Controller::stopCardEventMonitorThread()
225-
{
226-
if (cardEventMonitorThreadKey) {
227-
try {
228-
auto cardEventMonitorThread = childThreads.at(cardEventMonitorThreadKey);
229-
cardEventMonitorThreadKey = 0;
230-
if (cardEventMonitorThread) {
231-
interruptThread(cardEventMonitorThread);
232-
}
233-
} catch (const std::out_of_range&) {
234-
qWarning() << "Card event monitor thread" << cardEventMonitorThreadKey
235-
<< "is missing from childThreads map in stopCardEventMonitorThread()";
236-
cardEventMonitorThreadKey = 0;
237-
}
238-
}
239-
}
240-
241195
void Controller::disposeUI()
242196
{
243-
if (window) {
244-
window->disconnect();
245-
// As the Qt::WA_DeleteOnClose flag is set, the dialog is deleted automatically.
246-
window->close();
247-
window = nullptr;
248-
}
197+
delete window;
198+
window = nullptr;
249199
}
250200

251201
void Controller::onConfirmCommandHandler(const EidCertificateAndPinInfo& certAndPinInfo)
252202
{
253-
stopCardEventMonitorThread();
203+
emit stopCardEventMonitorThread();
254204

255205
try {
256-
CommandHandlerConfirmThread* commandHandlerConfirmThread =
206+
auto* commandHandlerConfirmThread =
257207
new CommandHandlerConfirmThread(this, *commandHandler, window, certAndPinInfo);
258208
connect(commandHandlerConfirmThread, &CommandHandlerConfirmThread::completed, this,
259209
&Controller::onCommandHandlerConfirmCompleted);
260-
saveChildThreadPtrAndConnectFailureFinish(commandHandlerConfirmThread);
210+
connect(commandHandlerConfirmThread, &ControllerChildThread::failure, this,
211+
&Controller::onCriticalFailure);
261212
connectRetry(commandHandlerConfirmThread);
262213

263214
commandHandlerConfirmThread->start();
@@ -373,18 +324,15 @@ void Controller::exit()
373324

374325
void Controller::waitForChildThreads()
375326
{
376-
// Waiting for child threads must not happen in destructor.
377-
// See https://tombarta.wordpress.com/2008/07/10/gcc-pure-virtual-method-called/ for details.
378-
for (const auto& childThread : childThreads) {
379-
auto thread = childThread.second;
380-
if (thread) {
381-
interruptThread(thread);
382-
// Waiting for PIN input on PIN pad may take a long time, call processEvents() so that
383-
// the UI doesn't freeze.
384-
while (thread->isRunning()) {
385-
thread->wait(100); // in milliseconds
386-
QCoreApplication::processEvents();
387-
}
327+
for (auto* thread : findChildren<QThread*>()) {
328+
qDebug() << "Interrupting thread" << uintptr_t(thread);
329+
thread->disconnect();
330+
thread->requestInterruption();
331+
ControllerChildThread::waitForControllerNotify.wakeAll();
332+
// Call processEvents() so that the UI doesn't freeze.
333+
while (thread->isRunning()) {
334+
thread->wait(100ms);
335+
QCoreApplication::processEvents();
388336
}
389337
}
390338
}

‎src/controller/controller.hpp‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,8 @@ class Controller : public QObject
4141

4242
signals:
4343
void quit();
44-
void statusUpdate(const RetriableError status);
44+
void statusUpdate(RetriableError status);
45+
void stopCardEventMonitorThread();
4546

4647
public: // slots
4748
void run();
@@ -76,18 +77,13 @@ class Controller : public QObject
7677
void runCommandHandler(const std::vector<electronic_id::ElectronicID::ptr>& availableEids);
7778
void connectOkCancelWaitingForPinPad();
7879
void connectRetry(const ControllerChildThread* childThread);
79-
void saveChildThreadPtrAndConnectFailureFinish(ControllerChildThread* childThread);
80-
void stopCardEventMonitorThread();
8180
void disposeUI();
8281
void exit();
8382
void waitForChildThreads();
8483
CommandType commandType();
8584

8685
CommandWithArgumentsPtr command;
8786
CommandHandler::ptr commandHandler = nullptr;
88-
std::unordered_map<uintptr_t, observer_ptr<ControllerChildThread>> childThreads;
89-
// Key of card event monitor thread in childThreads map.
90-
uintptr_t cardEventMonitorThreadKey = 0;
9187
// As the Qt::WA_DeleteOnClose flag is set, the dialog is deleted automatically.
9288
observer_ptr<WebEidUI> window = nullptr;
9389
QVariantMap _result;

‎src/controller/threads/cardeventmonitorthread.hpp‎

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -33,15 +33,15 @@ class CardEventMonitorThread : public ControllerChildThread
3333
using eid_ptr_vector = std::vector<electronic_id::ElectronicID::ptr>;
3434

3535
CardEventMonitorThread(QObject* parent, std::string commandType) :
36-
ControllerChildThread(parent), cmdType(std::move(commandType))
36+
ControllerChildThread(std::move(commandType), parent)
3737
{
3838
}
3939

4040
void run() override
4141
{
4242
QMutexLocker lock {&controllerChildThreadMutex};
4343

44-
beforeRun();
44+
qDebug() << "Starting" << metaObject()->className() << uintptr_t(this) << "for command" << commandType();
4545

4646
auto initialCards = getSupportedCardsIgnoringExceptions();
4747
sortByReaderNameAndAtr(initialCards);
@@ -57,7 +57,7 @@ class CardEventMonitorThread : public ControllerChildThread
5757
sortByReaderNameAndAtr(updatedCards);
5858
} catch (const std::exception& error) {
5959
// Ignore smart card layer errors, they will be handled during next card operation.
60-
qWarning() << className << "ignoring" << commandType() << "error:" << error;
60+
qWarning() << metaObject()->className() << "ignoring" << commandType() << "error:" << error;
6161
}
6262

6363
// If interruption was requested during wait, exit without emitting.
@@ -67,7 +67,7 @@ class CardEventMonitorThread : public ControllerChildThread
6767

6868
// If there was a change in connected supported cards, exit after emitting a card event.
6969
if (!areEqualByReaderNameAndAtr(initialCards, updatedCards)) {
70-
qDebug() << className << "card change detected";
70+
qDebug() << metaObject()->className() << "card change detected";
7171
emit cardEvent();
7272
return;
7373
}
@@ -90,7 +90,7 @@ class CardEventMonitorThread : public ControllerChildThread
9090
return electronic_id::availableSupportedCards();
9191
} catch (const std::exception& error) {
9292
// Ignore smart card layer errors, they will be handled during next card operation.
93-
qWarning() << className << "ignoring" << commandType() << "error:" << error;
93+
qWarning() << metaObject()->className() << "ignoring" << commandType() << "error:" << error;
9494
}
9595
waitForControllerNotify.wait(&controllerChildThreadMutex, ONE_SECOND);
9696
}
@@ -118,8 +118,4 @@ class CardEventMonitorThread : public ControllerChildThread
118118
&& c1->smartcard().atr() == c2->smartcard().atr();
119119
});
120120
}
121-
122-
const std::string& commandType() const override { return cmdType; }
123-
124-
const std::string cmdType;
125121
};

‎src/controller/threads/commandhandlerconfirmthread.hpp‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,8 @@ class CommandHandlerConfirmThread : public ControllerChildThread
3131
public:
3232
CommandHandlerConfirmThread(QObject* parent, CommandHandler& handler, WebEidUI* w,
3333
const EidCertificateAndPinInfo& certAndPin) :
34-
ControllerChildThread(parent), commandHandler(handler),
35-
cmdType(commandHandler.commandType()), window(w), certAndPinInfo(certAndPin)
34+
ControllerChildThread(handler.commandType(), parent), commandHandler(handler), window(w),
35+
certAndPinInfo(certAndPin)
3636
{
3737
}
3838

@@ -47,10 +47,7 @@ class CommandHandlerConfirmThread : public ControllerChildThread
4747
emit completed(result);
4848
}
4949

50-
const std::string& commandType() const override { return cmdType; }
51-
5250
CommandHandler& commandHandler;
53-
const std::string cmdType;
5451
WebEidUI* window;
5552
EidCertificateAndPinInfo certAndPinInfo;
5653
};

‎src/controller/threads/commandhandlerrunthread.hpp‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,7 @@ class CommandHandlerRunThread : public ControllerChildThread
3131
public:
3232
CommandHandlerRunThread(QObject* parent, CommandHandler& handler,
3333
const std::vector<electronic_id::ElectronicID::ptr>& eids) :
34-
ControllerChildThread(parent), commandHandler(handler),
35-
cmdType(commandHandler.commandType()), eids(eids)
34+
ControllerChildThread(handler.commandType(), parent), commandHandler(handler), eids(eids)
3635
{
3736
// Connect retry signal to retry signal to pass it up from the command handler.
3837
connect(&commandHandler, &CommandHandler::retry, this, &ControllerChildThread::retry);
@@ -41,9 +40,6 @@ class CommandHandlerRunThread : public ControllerChildThread
4140
private:
4241
void doRun() override { commandHandler.run(eids); }
4342

44-
const std::string& commandType() const override { return cmdType; }
45-
4643
CommandHandler& commandHandler;
47-
const std::string cmdType;
4844
std::vector<electronic_id::ElectronicID::ptr> eids;
4945
};

‎src/controller/threads/controllerchildthread.hpp‎

Lines changed: 18 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -27,10 +27,15 @@
2727
#include "qeid.hpp"
2828
#include "logging.hpp"
2929

30+
#include <QCoreApplication>
3031
#include <QThread>
3132
#include <QMutexLocker>
3233
#include <QWaitCondition>
3334

35+
#include <chrono>
36+
37+
using namespace std::chrono;
38+
3439
class ControllerChildThread : public QThread
3540
{
3641
Q_OBJECT
@@ -40,11 +45,11 @@ class ControllerChildThread : public QThread
4045
{
4146
QMutexLocker lock {&controllerChildThreadMutex};
4247

43-
beforeRun();
48+
qDebug() << "Starting" << metaObject()->className() << uintptr_t(this) << "for command" << commandType();
4449

4550
try {
4651
doRun();
47-
qInfo() << className << uintptr_t(this) << "for command" << commandType()
52+
qInfo() << metaObject()->className() << uintptr_t(this) << "for command" << commandType()
4853
<< "completed successfully";
4954

5055
} catch (const CommandHandlerVerifyPinFailed& error) {
@@ -89,31 +94,27 @@ class ControllerChildThread : public QThread
8994
void failure(const QString& error);
9095

9196
protected:
92-
explicit ControllerChildThread(QObject* parent) : QThread(parent) {}
93-
~ControllerChildThread() override
97+
explicit ControllerChildThread(std::string _cmdType, QObject* parent) :
98+
QThread(parent), cmdType(std::move(_cmdType))
9499
{
95-
// Avoid throwing in destructor.
96-
try {
97-
qDebug() << className << uintptr_t(this) << "destroyed";
98-
} catch (...) {
99-
}
100+
// When the thread is finished call deleteLater() on it to free the thread object. Although
101+
// the thread objects are freed through the Qt object tree ownership system anyway, it is
102+
// better to delete them immediately when they finish.
103+
connect(this, &ControllerChildThread::finished, this, [this] {
104+
qDebug() << metaObject()->className() << uintptr_t(this) << "finished";
105+
deleteLater();
106+
});
100107
}
101108

102-
void beforeRun()
103-
{
104-
// Cannot use virtual calls in constructor, have to initialize the class name here.
105-
className = metaObject()->className();
106-
qDebug() << "Starting" << className << uintptr_t(this) << "for command" << commandType();
107-
}
109+
const std::string& commandType() const { return cmdType; }
108110

109111
static const unsigned long ONE_SECOND = 1000;
110112

111113
static QMutex controllerChildThreadMutex;
112-
QString className;
113114

114115
private:
115116
virtual void doRun() = 0;
116-
virtual const std::string& commandType() const = 0;
117+
const std::string cmdType;
117118

118119
void warnAndEmitRetry(const RetriableError errorCode, const std::exception& error)
119120
{

0 commit comments

Comments
 (0)