D10824: Delete IdleSlave having temporary authorization

This revision was automatically updated to reflect the committed changes. Closed by commit R303:bbf120b78773: Delete IdleSlave having temporary authorization (authored by chinmoyr). REPOSITORY R303 KInit CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10824?vs=28536&id=31255 REVISION

D10824: Delete IdleSlave having temporary authorization

dfaure accepted this revision. dfaure added a comment. This revision is now accepted and ready to land. OK. If the slave indeed gets killed then this is what we want, indeed. REPOSITORY R303 KInit BRANCH master REVISION DETAIL https://phabricator.kde.org/D10824 To: chinmoyr, dfaure, #

D10824: Delete IdleSlave having temporary authorization

chinmoyr added a comment. David, what about this patch? It seems to me deleting the IdleSlave object is the only way to kill the ioslave from here. REPOSITORY R303 KInit REVISION DETAIL https://phabricator.kde.org/D10824 To: chinmoyr, dfaure, #frameworks Cc: #frameworks, michaelh, ngrah

D10824: Delete IdleSlave having temporary authorization

chinmoyr added a comment. I haven't yet tried your patch (which btw involves the http ioslave right?) but I am quite sure that exit() is not called for file ioslave after interrupting the application. I have placed the debug statements inside exit() as well as before every exit() call and I

D10824: Delete IdleSlave having temporary authorization

dfaure added a comment. I tested what happens in the current code when an idle job is killed. My testcase was configuring to use KIO in componentchooser ("open http urls in an application based on the contents on the URL") and then `kioclient5 exec www.kde.org`. This puts the slave on hold w

D10824: Delete IdleSlave having temporary authorization

chinmoyr added a comment. Neither SlaveBase::exit nor Slave::kill is called. I also commented the line in ConnectionBackend which removes the socket file but even then the slave process terminated. At this point I have no idea why is this working. REPOSITORY R303 KInit REVISION DETAIL

D10824: Delete IdleSlave having temporary authorization

dfaure added a comment. Interesting. But are you sure that neither Slave::kill() nor SlaveBase::exit() is called? It can't just be the deleteLater that does this, unless I'm missing something (e.g. closing the pipe makes the slave die with SIGPIPE maybe) REPOSITORY R303 KInit REVISION DE

D10824: Delete IdleSlave having temporary authorization

chinmoyr added a comment. Actually deleteLater() does kill the slave. I placed a qDebug statement in there and immediately after the message I am getting kdeinit5: PID $slave_pid terminated. REPOSITORY R303 KInit REVISION DETAIL https://phabricator.kde.org/D10824 To: chinmoyr, df

D10824: Delete IdleSlave having temporary authorization

dfaure added a comment. No, that deletes the IdleSlave QObject, but the slave process is still running then. IdleSlave::~IdleSlave() { } Not much happening there ;) Yes, that makes me wondering if the SLAVE_MAX_IDLE thing actually works. Hmm, and KLauncher::reques

D10824: Delete IdleSlave having temporary authorization

chinmoyr added inline comments. INLINE COMMENTS > dfaure wrote in klauncher.cpp:1114 > This deletes the Slave C++ class, but it doesn't actually kill the ioslave. > So why do it? > > I'm confused now. Do you want to kill the ioslave (then call slave->kill()) > or do you want to reuse that ioslav

D10824: Delete IdleSlave having temporary authorization

dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > klauncher.cpp:1114 > +mSlaveList.removeAll(slave); > +slave->deleteLater(); > +} This deletes the Slave C++ class, but it doesn't actually

D10824: Delete IdleSlave having temporary authorization

chinmoyr updated this revision to Diff 28536. chinmoyr added a comment. Used deleteLater() REPOSITORY R303 KInit CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10824?vs=28002&id=28536 BRANCH master REVISION DETAIL https://phabricator.kde.org/D10824 AFFECTED FILES src/klau

D10824: Delete IdleSlave having temporary authorization

dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. Wait, no, this is in a slot connected to the slave that we're deleting ==> you have to use deleteLater. REPOSITORY R303 KInit REVISION DETAIL https://phabricator.kde.org/D108

D10824: Delete IdleSlave having temporary authorization

dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R303 KInit BRANCH master REVISION DETAIL https://phabricator.kde.org/D10824 To: chinmoyr, dfaure, #frameworks Cc: #frameworks, michaelh

D10824: Delete IdleSlave having temporary authorization

lbeltrame added a reviewer: Frameworks. REPOSITORY R303 KInit REVISION DETAIL https://phabricator.kde.org/D10824 To: chinmoyr, dfaure, #frameworks Cc: #frameworks, michaelh

D10824: Delete IdleSlave having temporary authorization

chinmoyr created this revision. chinmoyr added a reviewer: dfaure. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: Frameworks. chinmoyr requested review of this revision. REVISION SUMMARY IdleSlave with temporary authorization can be easily misused.