Server application crashes when client is disconnected while an action is still running

[dwerner-d]

I'm currently facing a technical issue with the KDSOAP library (version 2.1.1). I've created a SOAP webservice application and I found out, that the service application crashes, when the client is disconnected while an action is still running.

The reason is the following connect command in KDSoapSocketList.cpp: QObject::connect(socket, &KDSoapServerSocket::disconnected, socket, &KDSoapServerSocket::deleteLater);

If the client is disconnected, the KDSoapServerSocket object will be destroyed. I guess normally this should happen only when the server action has finished. But if this action itself triggers the event loop of the application, the deletion is done immediatly and so the whole application crashes as soon as some data from the KDSoapServerSocket is accessed.

I can reproduce this issue by implementing the following server action:

    for (int i = 0; i < 500; ++i)
    {
        QCoreApplication::processEvents();
        QThread::msleep(10);
    }

When I close the client within these 5 seconds, the server will crash. I've already tried a workaround by implementing a customized deleteLater method, that delays the deleteLater call until the KDSoapServerSocket::slotReadyRead() method has finished. But maybe you have some smarter solutions for my issue.

[dfaure-kdab]

Thanks for the report and patch.

As you probably know, nested event loops are evil and the source of all sorts of problems - like this one ;) I will take a look at the fix, but generally speaking, I strongly recommend against nested event loops.

An easy solution at the level of your own server implementation would be to decouple the handling of the server action from the running of the event loop, via a QTimer::singleShot or an invokeMethod(QueuedConnection), wouldn't it? It would have the benefit of not only fixing the issue of the socket deletion, but also it would avoid any nasty re-entrancy into the kdsoap server code if another request comes in while being in that nested event loop.

[dwerner-d]

Thanks for your immediate response.

I have already faced lots of evil things caused by "creative" usage of event loops, so I'm very familiar with this topic. My sample code with processEvents was just to show the basic problem - I didn't implement anything like this. But I have to use an external API within my service application and this API internally somehow triggers the event loop. And I need to use the return value of this API call to set the return value of the service method. So I don't see any possibibility to decouple the action or call it somehow asynchronously.

Maybe this explanation helps to understand why I implemented the patch this way.

[dfaure-kdab]

I see (and yes it was clear that the sample code was just for reproducing the issue).

I think I have a better solution for you. KDSoapServerSocket supports delayed replies. You can call KDSoapServerSocket::setResponseDelayed(true), start the underlying async task, connect to a signal, and when the slot is called with information you need for the return value of the service method, you then call KDSoapServerSocket::sendDelayedReply.

This removes the need for a nested event loop, which fixes the problem of the server handling another request while in that nested event loop (KDSoapServerSocket disables reacting to other requests while waiting for the delayed reply).

[dwerner-d]

Thanks for your alternative solution, this seems to work.

Is there any convenient way to use the delayed responses by default for each service function?

I didn't find a global flag or something like that, so I implemented it manually for each function. All virtual methods now only contain the following code:

TNS__FunctionResponse QcFunctionService::function(const TNS__FunctionRequest& parameters)
{
    _DelayedResponseHandle = prepareDelayedResponse();
    _ApiExecWatcher.setFuture(QtConcurrent::run(function, parameters));
    return {};
}

And I had to implement a static method for each virtual method to be able to call it asynchronously with QtConcurrent.

It's working but since the web service provides ~100 functions, the code seems a bit messy.

[dfaure-kdab]

I'm very confused now, because the point of my last suggestion was to use an async job/task in the Qt event loop, not to use a secondary thread. This seems like a recipe for data races, unless you analyzed what data is now being accessed by multiple threads and needs mutex protection.

Looking at your comment again, though, I see "this [external] API internally somehow triggers the event loop". If that external library doesn't offer a way to do things async, and always opens up a nested event loop, then that's very broken design on their part.

Re your first question: no, there's no way to ask kdwsdl2cpp to generate exactly the code you need in the implementation, it would be different for every user of kdsoap (and I certainly don't want to hide/encapsulate running user code in a different thread, people would end up with tons of data races without even realizing). You could however hack up kdwsdl2cpp to generate what you need, I suppose.

[dwerner-d]

Thanks again for your response. I took care about possible race conditions and the solution is working stable. From my point of view the pull request is not needed anymore.