IdleSlave with temporary authorization can be easily misused. So delete any such IdleSlave.
Depends on D10822
dfaure |
Frameworks |
IdleSlave with temporary authorization can be easily misused. So delete any such IdleSlave.
Depends on D10822
Automatic diff as part of commit; lint not applicable. |
Automatic diff as part of commit; unit tests not applicable. |
Wait, no, this is in a slot connected to the slave that we're deleting ==> you have to use deleteLater.
src/klauncher/klauncher.cpp | ||
---|---|---|
1114 | This deletes the Slave C++ class, but it doesn't actually kill the ioslave. I'm confused now. Do you want to kill the ioslave (then call slave->kill()) |
src/klauncher/klauncher.cpp | ||
---|---|---|
1114 | I meant to kill the slave and not reuse it. |
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::requestHoldSlave looks like it's leaking the IdleSlave (well, child of "this", but the object won't be deleted until logging out).
Unless I'm missing something, that's a bug from 2001 :-)
Anyhow, one thing at a time, can you test if the kioslave process really goes away?
My bet is that it doesn't, in which case you need a call to KIOPrivate::sendTerminateSignal(slave->pid());
(if that's not available from this repository, add a method IdleSlave::kill())
[The alternative solution is to call SlaveBase::exit() from the slave process itself, but there's no good place for doing that I think, so better kill it when klauncher decides it's idle, it's simpler than sending it a command to ask it to exit.]
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.
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)
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.
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 while it emits the mimetype, while KIO runs the associated application for that mimetype.
The idea was that the app can then use that job to resume the same transfer, if the app uses KIO. In my testcase it's launching konqueror+webenginepart, which doesn't use KIO, so the slave remains idle and gets killed later by klauncher.
What happens just after KLauncher::idleTimeout deletes the slave is that the slave goes to this code path inside mimeType() :
if (ret == -1) { qDebug() << "read error"; exit(); }
So in my testcase at least, it goes to SlaveBase::exit(), which is how the slave exits :)
You could try the same set of debugging statements in your testcase... http://www.davidfaure.fr/2018/exit_debug.diff for kio and http://www.davidfaure.fr/2018/kinit.diff for kinit.
Remember to restart kdeinit5 afterwards by typing kdeinit5 in a terminal - but I guess you know this already ;-)
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 am not seeing any output.
That aside here's what I have observed till now,
After IdleSlave is created, inside dispatchLoop() SlaveBase waits in this line
if (d->appConnection.hasTaskAvailable() || d->appConnection.waitForIncomingTask(ms)) {
with appConnection having no tasks available.
After deleting the IdleSlave waitForIncomingTask returns false because the Connection object in IdleSlave was destroyed which interrupted the connection and changed the socket state from QAbstractSocket::ConnectedState (It's merely a speculation. I haven't verified it yet). Then the variable ret is assigned -1. Since the connection was interrupted, SlaveBase returns from dispatchLoop and in FileProtocol kdemain returns thus terminating the process.
Please fill me in if you think I have missed something.
David, what about this patch? It seems to me deleting the IdleSlave object is the only way to kill the ioslave from here.