Skip to content

Commit a7be1ec

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

File tree

7 files changed

+73
-139
lines changed

7 files changed

+73
-139
lines changed

‎src/controller/controller.cpp‎

Lines changed: 35 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();
@@ -351,6 +302,7 @@ void Controller::onPinPadCancel()
351302

352303
void Controller::onCriticalFailure(const QString& error)
353304
{
305+
emit stopCardEventMonitorThread();
354306
qCritical() << "Exiting due to command" << std::string(commandType())
355307
<< "fatal error:" << error;
356308
_result =
@@ -373,18 +325,15 @@ void Controller::exit()
373325

374326
void Controller::waitForChildThreads()
375327
{
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-
}
328+
for (auto* thread : findChildren<QThread*>()) {
329+
qDebug() << "Interrupting thread" << uintptr_t(thread);
330+
thread->disconnect();
331+
thread->requestInterruption();
332+
ControllerChildThread::waitForControllerNotify.wakeAll();
333+
// Call processEvents() so that the UI doesn't freeze.
334+
while (thread->isRunning()) {
335+
thread->wait(100ms);
336+
QCoreApplication::processEvents();
388337
}
389338
}
390339
}

‎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: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -33,15 +33,16 @@ 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"
45+
<< commandType();
4546

4647
auto initialCards = getSupportedCardsIgnoringExceptions();
4748
sortByReaderNameAndAtr(initialCards);
@@ -57,7 +58,8 @@ class CardEventMonitorThread : public ControllerChildThread
5758
sortByReaderNameAndAtr(updatedCards);
5859
} catch (const std::exception& error) {
5960
// Ignore smart card layer errors, they will be handled during next card operation.
60-
qWarning() << className << "ignoring" << commandType() << "error:" << error;
61+
qWarning() << metaObject()->className() << "ignoring" << commandType()
62+
<< "error:" << error;
6163
}
6264

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

6870
// If there was a change in connected supported cards, exit after emitting a card event.
6971
if (!areEqualByReaderNameAndAtr(initialCards, updatedCards)) {
70-
qDebug() << className << "card change detected";
72+
qDebug() << metaObject()->className() << "card change detected";
7173
emit cardEvent();
7274
return;
7375
}
@@ -90,7 +92,8 @@ class CardEventMonitorThread : public ControllerChildThread
9092
return electronic_id::availableSupportedCards();
9193
} catch (const std::exception& error) {
9294
// Ignore smart card layer errors, they will be handled during next card operation.
93-
qWarning() << className << "ignoring" << commandType() << "error:" << error;
95+
qWarning() << metaObject()->className() << "ignoring" << commandType()
96+
<< "error:" << error;
9497
}
9598
waitForControllerNotify.wait(&controllerChildThreadMutex, ONE_SECOND);
9699
}
@@ -110,16 +113,10 @@ class CardEventMonitorThread : public ControllerChildThread
110113

111114
static bool areEqualByReaderNameAndAtr(const eid_ptr_vector& a, const eid_ptr_vector& b)
112115
{
113-
// std::equal requires that second range is not shorter than first, so compare size first.
114-
return a.size() == b.size()
115-
&& std::equal(a.cbegin(), a.cend(), b.cbegin(),
116+
return std::equal(a.cbegin(), a.cend(), b.cbegin(), b.cend(),
116117
[](const eid_ptr& c1, const eid_ptr& c2) {
117118
return c1->smartcard().readerName() == c2->smartcard().readerName()
118119
&& c1->smartcard().atr() == c2->smartcard().atr();
119120
});
120121
}
121-
122-
const std::string& commandType() const override { return cmdType; }
123-
124-
const std::string cmdType;
125122
};

‎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
};

0 commit comments

Comments
 (0)