mutex/semaphore wait blocks a slot

Chris Kawa

Do I have to rig something up to make the GET call synchronous?

No, definitely not. I'd say get rid of any synchronization (especially blocking ones) you can.

Or, is there another way to go about this?

It depends on what were you trying to achieve with the mutex in the first place. Ordering of requests? Ordering of the slots?

mzimmers

@Chris-Kawa said in mutex/semaphore wait blocks a slot:

It depends on what were you trying to achieve with the mutex in the first place. Ordering of requests? Ordering of the slots?

In the unlikely event that the server is slow in producing a response to me, I want to avoid sending a second GET until I get a reply to my first one.

I use this method in several places in my app, and it seems to work fine. This particular example is kind of a special case, because I definitely will be trying to make two calls in rapid succession. (The reason for this is at startup, my location is unknown. Once I get my location, I want to get a new weather code.)

Chris Kawa

@mzimmers said:

I use this method in several places in my app, and it seems to work fine

It works fine on your network. This is a terrible terrible way of doing this. Very manual and error prone.

If you want a request queue make a request queue - create a vector of requests. Push a request to it and signal. Have a processor that waits for that signal. If there's something in flight already do nothing, if there isn't pop a request from the vector and post it. When a slot arrives or request times out signal the processor again to check if there's more requests to post in the queue.

mzimmers

@Chris-Kawa said in mutex/semaphore wait blocks a slot:

OK, I think I can do that. Everything you said makes sense to me except:

Have a processor that waits for that signal

Not sure what you mean by "processor" - do you mean a handler/slot?

Chris Kawa

@mzimmers said in mutex/semaphore wait blocks a slot:

Not sure what you mean by "processor" - do you mean a handler/slot?

Yup, something that processes the queue. Can be just a slot or you could encapsulate the queue and its handling in a dedicated class.

mzimmers

@Chris-Kawa I'm chasing my tail on this. Is it possible that part of my problem is that I'm attempting to do everything from a single thread?

Here's my network code, as minimal as I can make it:

requestWeather(1);
requestWeather(2);
requestWeather(3);

void SystemModel::requestWeather(int i)
{
    QUrlQuery query; 
    // fill out query
    QObject::connect(m_messageMgr, &MessageManager::receivedWeather, this, &SystemModel::processWeatherReply);
    m_messageMgr->addWeatherRequest(query);
}

void MessageManager::addWeatherRequest(QUrlQuery query)
{
    QNetworkRequest l_request;
    // fill out l_request
    // add to the list.
    m_weatherRequestList.append(l_request);
    processList();
}

void MessageManager::processList() {
    do {
        if (m_weatherReply != nullptr) {
            qWarning() << __PRETTY_FUNCTION__ << "m_weatherReply != nullptr; not attempting to process list.";
            continue;
        }
        while (!m_weatherRequestList.isEmpty()) {
            QNetworkRequest request = m_weatherRequestList.front();
            m_weatherReply = m_manager.get(request);
            if (m_weatherReply->error() != QNetworkReply::NoError) {
                continue;
            }
            QObject::connect(m_weatherReply, &QNetworkReply::finished, this, &MessageManager::notifyRequester);
            m_weatherRequestList.removeFirst();
        }
    } while (false);
}

void MessageManager::notifyRequester() {
    emit receivedWeather(m_weatherReply);
    m_weatherReply->deleteLater();
}

My slot processWeatherReply() isn't called until all three of the calls to requestWeather() have exited. I can provide timestamped telltales if desired.

Chris Kawa

@mzimmers The problem is you're trying to empty the whole queue at the same time (while (!m_weatherRequestList.isEmpty()) ), which defeats the purpose of a queue. Don't do that.

Remove both loops from processList. Pop just the first request (if the list is not empty), post it and that's it, exit the function. In the notifyRequester callback, after you finish processing the reply, call processList again, so that it posts another request. This will keep happening until the list is emptied.

mzimmers

@Chris-Kawa well, progress of sorts. Here's my cleaned up code (along with the slot):

void MessageManager::processList() {
    if (!m_weatherRequestList.isEmpty()) {
        QNetworkRequest request = m_weatherRequestList.front();
        m_weatherReply = m_manager.get(request);
        QObject::connect(m_weatherReply, &QNetworkReply::finished, this, &MessageManager::notifyRequester);
        m_weatherRequestList.removeFirst();
    }
}

void MessageManager::notifyRequester() {
    emit receivedWeather(m_weatherReply);
    m_weatherReply->deleteLater();
    processList();
}

void SystemModel::processWeatherReply(QNetworkReply *qnr) {
    qba = qnr->readAll(); // only the first read returns anything.
    // lots more stuff
    qnr->disconnect();
    qnr->deleteLater();
    qnr = nullptr;
}

So, the three calls to requestWeather() all complete before the first slot is called, but I suppose that's to be expected now because that routine is doing so little work. (yes?)

REMAINING ISSUE:

The first transaction works correctly and returns the expected payload. Subsequent calls, however, return nothing from readAll().

I tried inserting a 1 second delay between the calls, just in case the server had some kind of DDoS protection, ~~but that didn't change anything.~~

EDIT:

The one-second delay did indeed help; now all three calls return valid payload. Now the problem is I'm getting an extra signal from the message manager, but I'll dig into that and report back.

mzimmers

Well, I think this one is mostly solved. For some reason, my processWeatherReply() slot is getting called more often than its only signal is emitted, which is...weird. The "extra" calls to the slot are using the same qnr as the preceding "good" call, but the readAll() returns nothing.

I can easily code around this issue, though I would like to know how I'm getting those extra calls. Some kind of weird EOL that's not getting picked up in the first read, maybe?

Chris Kawa

@mzimmers Don't insert artificial delays. It's one second on your machine and might be 10 on mine.

You removed too much from processList i.e. the check to see if something is already in flight: if (m_weatherReply != nullptr). That needs to stay, because now you're, again, posting multiple requests at the same time and overwriting m_weatherReply, which defeats the purpose of a queue and the reason you're not getting anything from readAll. You're reading multiple times from the same overwritten reply.

Add back the check and exit if m_weatherReply is not null. Remember to set it to null after you call deleteLater() on it, otherwise you'll get a stale pointer in the processList() call that follows.

You're also deleting the reply twice, once in notifyRequester and once in processWeatherReply. It works by accident, since you're not getting back to the event loop inbetween, but that's a bug. Delete it once in notifyRequester. You also don't need to disconnect it beforehand. Deleting an object breaks all of its connections automatically.

mzimmers

@Chris-Kawa

I was only using the delays to test whether part of my problem was a DDoS defense (and I think it was). The multiple calls, and the delays between them, have been removed.

I'm still getting some empty reads. Here's my updated network code:

void MessageManager::addWeatherRequest(QUrlQuery query)
{
    QNetworkRequest l_request;

    // add to the list.
    m_weatherRequestList.append(l_request);
    processList();
}

void MessageManager::processList() {
    if (m_weatherReply == nullptr) {
        if (m_weatherRequestList.isEmpty()) {
            qDebug() << __PRETTY_FUNCTION__ << "nothing in list to process.";
        } else {
            QNetworkRequest request = m_weatherRequestList.front();
            m_weatherReply = m_manager.get(request);
            if (m_weatherReply->error() == QNetworkReply::NoError) {
                QObject::connect(m_weatherReply, &QNetworkReply::finished, this, &MessageManager::notifyRequester);
            } 
            m_weatherRequestList.removeFirst();
        }
    } else {
        qWarning() << __PRETTY_FUNCTION__ << "attempting to use m_weatherReply which is already in use.";
    }
}

void MessageManager::notifyRequester() {
    qint64 bytesAvailable = m_weatherReply->bytesAvailable();
    if (bytesAvailable > 0) {
        qDebug() << __PRETTY_FUNCTION__ << "emitting receivedWeather().";
        emit receivedWeather(m_weatherReply);
    }
    m_weatherReply->deleteLater();
    m_weatherReply = nullptr;
    processList();
}

I've verified that the signal is only emitted twice now (which is correct) but I'm entering my slot more often (and getting those empty reads).

Chris Kawa

@mzimmers Well, you do have this in your requestWeather call:

QObject::connect(m_messageMgr, &MessageManager::receivedWeather, this, &SystemModel::processWeatherReply);

and it connects the same things every time, so you have three connections after three calls. When they finish you'll get 3 slot invocations for each, 9 in total. Either do that connection somewhere else once or add Qt::UniqueConnection as the 5th argument to connect, so that you don't get multiple connections for the same arguments.

mzimmers

@Chris-Kawa bingo! Good catch there - I didn't realize that you could "stack" connections; I thought any additional, identical connections were merely superfluous. I moved that connection to the c'tor of my SystemModel object, and it works fine.

As a postscript to this exercise...does my approach now make sense in general, or is this still an undesirable way to go about this?

Chris Kawa

@mzimmers said:

does my approach now make sense in general, or is this still an undesirable way to go about this?

It's a valid solution. Whether or not it's best for your case is something you have to answer yourself.