invokeRemoteMethodBlocking invokes slots (causing recursion)

Issue #10 resolved
Former user created an issue

Hello

I discovered that invokeRemoteMethodBlocking will process signals, which causes all kinds of problems when called from a widget.

I'm implementing a remote item model right now, which looks like this:

  virtual int rowCount(const QModelIndex & parent = QModelIndex()) const {
    QJsonRpcMessage response = mySocket->invokeRemoteMethodBlocking(myServiceName + ".rowCount", myInstanceId, parent.row(), parent.column());
    return response.result().toInt();
  }

But this causes further signals to be processed, which ultimately leads to a crash when a paint event gets dispatched within the call.

Comments (4)

  1. Matt Broadstone

    Could you possibly post a somewhat more complete code example, so that I can make an auto test out of this?

  2. Abdullah Ali

    Couldn't separate it from my project (it's full of SSL layer/database code), so I wrote an isolated case that works on the same principles.

    Apparently I can't attach files with Bitbucket right now so I'll use Dropbox instead: https://dl.dropboxusercontent.com/u/27714141/qjsonrpc_test.zip

    This client crashes when QTableView::paint invokes the model, which calls invokeRemoteMethodBlocking to retrieve the remote data, at which point the nested QEventLoop kicks in and dispatches more events (including scheduled paint events) and so on.

  3. Matt Broadstone

    Hey, I spent a pretty significant amount of time trying to solve this for you, but I've got some bad news :|

    It actually turns out that the bug exhibited by this sample app (I reduced it even further in my local test, I can submit that back to you if you'd like) is just a bug in Qt's item views.. so that's heartening in a way, but also won't be fixed immediately or even soon.

    The bigger problem here is in the intent of using the blocking API for an item model in the first place. Honestly, I struggled with providing the blocking API in the first place, since it's generally just not a good idea block ever (in a gui app or in a server app), but it provided a very nice way to easily autotest the library. Blocking an item model in the gui thread is just going to be bad practice any way you look at it. Rather, a (imho) better design would be to provide a means of retrieving the data from the jsonrpc server and perhaps listen to signals from the service indicating that data has changed, and handle all the item model specifics client-side. Basically, the challenge is to redesign your system to "think" asynchronously, rather than depending on synchronous/blocking calls.

    Actually, some funny and useful feedback from the qtbase maintainer on this point:

    thiago | NEVER block the GUI thread
    thiago | avoid nested event loops at all costs
    thiago | those are the two most important rules of event loops in Qt
    thiago | you're violating the second rule and you're thinking of making it worse by violating
               the first one
    

    So, unfortunately, I'm inclined to not modify the codebase to fully block in the way you're looking to do it.

    If it's an absolute requirement that you use this design, I started on a POC showing how you might go about it asynchronously (if you REALLY have to mirror the QAIM over the wire), but it's pretty messy business. Essentially, you would have to cache all of the data locally (maybe with an internal QStandardItemModel, but I was only using a one-level-deep list model), and have a hash maintaining request state. So you would have something like this:

    bool RemoteModel::rowCount() const
    {
        Q_D(const RemoteModel);
        if (d->requestHash[RowCountRequest] == false) {
            QJsonRpcServiceReply *reply = d->socket->invokeMethod(....);
            d->requestHash[RowCountRequest] = true;
            // then, store the reply somewhere, connect its finished method to something that
            // updates the local (client-side) internal model, then sets 
            // requestHash[RowCountRequest] = false, and emits modelReset()
        }
    
        return d->internalModel->rowCount();
    }
    

    Like I said, it's preeetty messy, and also incredibly inefficient since you'll be caching the data in the internal model AND the actual item model you're implementing itself. Let me know if you'd like some help with this (or my sample).

    Having said all of that, you do have a solution that works, so by all means continue to use that in your fork.

    Good luck!

  4. Log in to comment