Important: Please read the Qt Code of Conduct - https://forum.qt.io/topic/113070/qt-code-of-conduct

Max requests at the same time with QNetworkAccessManager



  • I need to create a class that will hold a list of urls and when I call the start method it will start the requests for those urls, the thing is that I want to limit the concurrent urls to make the request. QNetworkAccessManager allows 6 concurrent requests at the same time but I often need to connect to websites that doesn't allow concurrent requests so I need to limit that to 3 requests at the same time.

    That is what I have so far:

    QueuedRequester::QueuedRequester(int concurrent, QObject *parent) : mConcurrent(concurrent), QObject(parent)
    {
        mManager.setRedirectPolicy(QNetworkRequest::NoLessSafeRedirectPolicy);
    }
    
    void QueuedRequester::setUrls(const QList<QUrl> &urls)
    {
        mUrls = urls;
    }
    
    void QueuedRequester::start()
    {
        // ...
    }
    
    void QueuedRequester::replyFinished()
    {
        auto reply = qobject_cast<QNetworkReply *>(sender());
        
        reply->deleteLater();
    }
    

    Any idea of how I can archive that?


  • Lifetime Qt Champion

    Hi,

    Didn't you already had a thread about that exact subject a few days ago ?



  • It's not the same thing, the one that I asked was with incoming urls, this one is with a list of urls.

    I tried to code but I'm getting errors related to index out of range:

    QueuedRequester::QueuedRequester(int concurrent, QObject *parent) : mConcurrent(concurrent), QObject(parent)
    {
        mManager.setRedirectPolicy(QNetworkRequest::NoLessSafeRedirectPolicy);
    }
    
    void QueuedRequester::setUrls(const QList<QUrl> &urls)
    {
        mUrls = urls;
    }
    
    void QueuedRequester::start()
    {
        for (int i = 0; i < mConcurrent; ++i) {
            nextUrl();
        }
    }
    
    void QueuedRequester::abort()
    {
        for (auto reply : mManager.findChildren<QNetworkReply *>()) {
            reply->abort();
        }
    }
    
    void QueuedRequester::nextUrl()
    {
        ++mCurrent;
    
        auto size = mUrls.count() - 1;
    
        if (mCurrent == size) {
            return finished();
        }
    
        qDebug() << mCurrent;
    
        auto url = mUrls.at(mCurrent);
        auto request = QNetworkRequest(url);
        auto reply = mManager.get(request);
    
        connect(reply, &QNetworkReply::finished, this, &QueuedRequester::replyFinished);
    }
    
    void QueuedRequester::replyFinished()
    {
        auto reply = qobject_cast<QNetworkReply *>(sender());
        reply->deleteLater();
    
        auto status = reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt();
        qDebug() << status;
    
        nextUrl();
    }
    

    I have 6 urls in the list for testing and I wanted to have 3 concurrent, for some reason it's trying to get the index 6 and 7 and is skipping the 5. The skipping is happening cause the checking mCurrent == size is being made and the signal is emited, the problem is that the requests that is operating is still going to call nextUrl and for some reason it's passing the checking and trying to get 6 and 7. 😳


  • Moderators

    @Mr-Gisa said in Max requests at the same time with QNetworkAccessManager:

    if (mCurrent == size) {
    return finished();
    }

    It's because you have multiple places (i.e. the signal) that can call nextUrl and you increment mCurrent before testing it. So it's possible to get mCurrent > size, yet your check that I highlighted above only checks for equality.

    You can either change the check to mCurrent >= size) or you can do a test like if (mCurrent + 1 == size) then increment mCurrent after that size check.

    Also, no need for size variable because you only reference it once, so you might as well just inline it with mUrls.count()-1 and save on the cpu cycles of creating a variable.



  • @ambershark The changes you pointed out worked, the problem now is that it's emitting the signal three times.

    That is the output:

    0
    1
    2
    200
    3
    200
    4
    200
    finished
    200
    finished
    200
    finished

    That with the int mCurrent = -1 declared in the header, cause if I put 0, it will start at position 1 and skip the first item in the list.

    I feel like my code is a total spaghetti and it feels so bad :(


  • Moderators

    @Mr-Gisa said in Max requests at the same time with QNetworkAccessManager:

    That with the int mCurrent = -1 declared in the header, cause if I put 0, it will start at position 1 and skip the first item in the list.

    Nothing wrong with this. I've done that many times. Otherwise you have to have a special case for the 0 or you need to increment at the end rather than beginning. I usually do the latter, but there's nothing wrong with starting at -1 imo.

    I feel like my code is a total spaghetti and it feels so bad :(

    Learning what makes clean/sleek/nice/whatever/non-spaghetti code just takes time. The more you code the better you will get. Elegant solutions that you are looking for just take experience to think up. You'll get there, just keep at it.

    My thoughts on what you're doing .... I would have a list of urls that you remove the url from while processing. That way you can check for an empty list to know when you're done. This also means you only signal your finished() once since you can do a check like if (myList.isEmpty()) finished(); or something like that.

    A typical queue, would have you pushing and popping items off of a queue in order to process them. You could still limit to say 3 going at any given time.



  • @ambershark said in Max requests at the same time with QNetworkAccessManager:

    My thoughts on what you're doing .... I would have a list of urls that you remove the url from while processing. That way you can check for an empty list to know when you're done.

    I have already previously suggested this to the OP for this issue, but the thread seems to have been deleted by someone....


  • Moderators

    @JonB said in Max requests at the same time with QNetworkAccessManager:

    @ambershark said in Max requests at the same time with QNetworkAccessManager:

    My thoughts on what you're doing .... I would have a list of urls that you remove the url from while processing. That way you can check for an empty list to know when you're done.

    I have already previously suggested this to the OP for this issue, but the thread seems to have been deleted by someone....

    The OP himself deleted the thread.

    @Mr-Gisa, when you receive the answer you need, please mark the topic as "Solved" instead of deleting it. This way, other people can find the posts in the future and benefit from the discussion.



  • @JonB @JKSH I deleted the topic myself and the reason is that it was confused even for myself. I created the topic asking for help with the algorithm but I saw that it was really confusing and it wasn't solved so I decided to create a topic explaining things better and with the code I have, this way when solved it would be better for me and for the others. I don't delete topics that are solved or at least the ones I find that is going to help others, which is the case of the deleted topic mentioned by you guys.

    0_1526563365066_ffaba9ca-704e-4e0e-bdcc-a8ed6946c8ae-image.png



  • @ambershark I made the changes you pointed out and it looks way better, the problem is still the same as the first one tho, it's emitting finished 3 times.

    QueuedRequester::QueuedRequester(int concurrent, QObject *parent) : mConcurrent(concurrent), QObject(parent)
    {
        mManager.setRedirectPolicy(QNetworkRequest::NoLessSafeRedirectPolicy);
    }
    
    void QueuedRequester::setUrls(const QList<QUrl> &urls)
    {
        mQueue.append(urls);
    }
    
    void QueuedRequester::start()
    {
        for (int i = 0; i < mConcurrent; ++i) {
            nextUrl();
        }
    }
    
    void QueuedRequester::abort()
    {
        for (auto reply : mManager.findChildren<QNetworkReply *>()) {
            reply->abort();
        }
    }
    
    void QueuedRequester::requestUrl(const QUrl &url)
    {
        auto request = QNetworkRequest(url);
        auto reply = mManager.get(request);
    
        connect(reply, &QNetworkReply::finished, this, &QueuedRequester::handlePage);
    }
    
    void QueuedRequester::nextUrl()
    {
        if (mQueue.empty()) {
            return finished();
        }
    
        requestUrl(mQueue.dequeue());
    }
    
    void QueuedRequester::handlePage()
    {
        auto reply = qobject_cast<QNetworkReply *>(sender());
        reply->deleteLater();
    
        auto status = reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt();
        qDebug() << status;
    
        nextUrl();
    }
    


  • @Mr-Gisa
    You are calling nextUrl(); in both QueuedRequester::start() & QueuedRequester::handlePage(). Why is that?

    Are the urls safely in mQueue (via setUrls()), and mCurrent correctly set correspondingly, by the time QueuedRequester::start() is called?

    QueuedRequester::nextUrl() emits finished(); each & every time the queue is empty, not just once. So you have to be real careful about how many times it's called.

    I don't know exactly, and I'm not going to follow each line of code, but it's a couple of things you can check. Maybe if you try with just a few Urls (not your 100), and print out what Url is for each start & finish, you'll spot where it's going wrong...?



  • @JonB

    You are calling nextUrl(); in both QueuedRequester::start() & QueuedRequester::handlePage(). Why is that?

    Because as I need to start the amount of requests that is defined in mConcurrents I need to call nextUrl in the loop.
    And in the handlePage because when a request is finished it will request the next one.

    Are the urls safely in mQueue (via setUrls()), and mCurrent correctly set correspondingly, by the time QueuedRequester::start() is called?

    mCurrent doesn't exist here anymore as I changed from QList to QQueue as @ambershark pointed out. And the urls are in the mQueue when added via setUrls.

    QueuedRequester::nextUrl() emits finished(); each & every time the queue is empty, not just once. So you have to be real careful about how many times it's called.

    In theory it should work, I mean, start 3 requests, when one is finished starts the next one, then we will always have 3 requests going on, until there's no more urls to request.
    The thing is that I have to check when there's no more urls and nextUrl is the only place that I thought.

    I don't know exactly, and I'm not going to follow each line of code, but it's a couple of things you can check. Maybe if you try with just a few Urls (not your 100), and print out what Url is for each start & finish, you'll spot where it's going wrong...?

    I'm doing with 6 urls and 3 concurrent, that is the output:

    200
    200
    200
    200
    finished
    200
    finished
    200
    finished

    It always emit finished the amount of mConcurrent, like when the last 3 requests are being made, when they finish, they try to see if there's more urls and it's empty and so it emit finished.


  • Qt Champions 2019

    @Mr-Gisa To not to emit finished() more than once you could use a boolean variable which is set to true when emitting finished(). And finished() is only emitted if it is false.


  • Moderators

    Hi,
    @Mr-Gisa said in Max requests at the same time with QNetworkAccessManager:

    void QueuedRequester::nextUrl()
    {
    if (mQueue.empty()) {
    return finished();
    }

    requestUrl(mQueue.dequeue());
    }

    I think the culprit is here.

    You only check if the mQueue is empty. Each of your concurrent runs is checking after its done it work, if the queue is empty or if there's still work to be done. 3 Concurrents 3 checks if empty 3 times result true 3 times finished signal.

    Simple solution, create a QVector<bool> m_NetworkrequestsOngoing each concurrent sets one state of the vector to true, when its doing work and back to false, when its done.

    Than check for mQueue.empty() and m_NetworkrequestsOngoing.contains(true)

    void QueuedRequester::nextUrl()
     {
         if (mQueue.empty() && !m_NetworkrequestsOngoing.contains(true) {
             return finished();
     }
    
       requestUrl(mQueue.dequeue());
     }
    


  • @J.Hilk I'm kinda confused with that, how am I going to know which request set to true in order to set it to false as it is a QVector and I can only insert a bool in it?


  • Moderators

    @Mr-Gisa What I meant when I talked about removing items from a list as you processed them was that when the list was empty you were truly finished. However you are removing the items before processing on the item is complete, so you are getting 3 finishes rather than just one because you are checking for an empty queue "to be processed" rather than a "finished being processed" list.

    There are a couple ways to handle this...

    1. You can create another list of "processing" items and when that list is empty emit the finish. So right before you process the item, push it on the list. Then when processing is finished or abort is called, pop it off the list. After the pop check the list for isEmpty and if it is you're truly finished (and only once).

    2. You can just use an int member in your class and when it hits 0 it's finished. When you start processing increment it by 1, when you finish processing decrement it. When it's 0, you're done, emit finished.


Log in to reply