Differential D8561

[d_ptr] Do not harsh destroy QThread
ClosedPublic
Actions

Authored by anthonyfieroni on Oct 30 2017, 9:07 PM.

Details

Reviewers
davidedmundson
ivan
Group Reviewers
Plasma
Commits
R161:27c0245b1715: [resources] Nicely quit threads
Summary

systemd makes coredumps in /var/lib/systemd/coredumps when kactivitymanagerd_plugin_sqlite.so quits

coredumpctl list
coredumpctl gdb PID
(gdb) bt
#0  0x00006d63f0d0513f in raise () from /lib/libc.so.6
#1  0x00006d63f0d0656a in abort () from /lib/libc.so.6
#2  0x00006d63f148792e in QMessageLogger::fatal(char const*, ...) const ()
   from /usr/lib/libQt5Core.so.5
#3  0x00006d63f14991bc in QThread::~QThread() () from /usr/lib/libQt5Core.so.5
#4  0x00006d63d17c6c8a in ?? ()
   from /usr/lib/qt5/plugins/kactivitymanagerd/1/kactivitymanagerd_plugin_sqlite.so
#5  0x00006d63f0d07a80 in __run_exit_handlers () from /lib/libc.so.6
#6  0x00006d63f0d07ada in exit () from /lib/libc.so.6
#7  0x00006d63f2dd5e75 in QXcbConnection::processXcbEvents() ()
---Type <return> to continue, or q <return> to quit---
   from /usr/lib/qt5/plugins/platforms/../../../libQt5XcbQpa.so.5
#8  0x00006d63f1699141 in QObject::event(QEvent*) () from /usr/lib/libQt5Core.so.5
Test Plan

Try to nice quit a thread

Diff Detail

Repository
R161 KActivity Manager Service
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
anthonyfieroni created this revision.Oct 30 2017, 9:07 PM
Restricted Application added a project: Plasma. · View Herald TranscriptOct 30 2017, 9:07 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
davidedmundson added a comment.Oct 30 2017, 9:28 PM

Wouldn't it be cleaner to put the quit in:

Resources::Private::~Private
and/or ResourceScoreMaintainer::Private::~Private

(also I have no idea what the Resource thread is for, literally the only thing it does actually in the thread is sleep)

anthonyfieroni updated this revision to Diff 21603.Oct 31 2017, 8:34 AM

Correct patch version.

anthonyfieroni added a comment.EditedOct 31 2017, 8:35 AM
In D8561#162062, @davidedmundson wrote:

Wouldn't it be cleaner to put the quit in:

Resources::Private::~Private
and/or ResourceScoreMaintainer::Private::~Private

(also I have no idea what the Resource thread is for, literally the only thing it does actually in the thread is sleep)

Yeah, it's clear to put in where private is QThread, but i think this way is more consistent.

davidedmundson added a comment.Oct 31 2017, 9:40 AM

consistent with what?

anthonyfieroni added a comment.Oct 31 2017, 2:02 PM

With metaprogramming approach.

ivan added a comment.Nov 3 2017, 9:35 AM

While I like the idea behind the patch, I'd rather have what David proposed. Namely, d_ptr is a smart pointer, but I don't think it should be this smart - creating specializations for different types' destructors should not really be a part of it.

davidedmundson accepted this revision.Nov 3 2017, 10:55 AM

Thanks for fixing

This revision is now accepted and ready to land.Nov 3 2017, 10:55 AM
Closed by commit R161:27c0245b1715: [resources] Nicely quit threads (authored by anthonyfieroni). · Explain WhyNov 3 2017, 9:33 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 21852

View Options

src/service/Resources.cpp

Loading...
View Options

src/service/Resources_p.h

Loading...
View Options

src/service/plugins/sqlite/ResourceScoreMaintainer.cpp

Loading...