[Solved] [QNetworkAccessManager] Downloads Won't Start

sammarks

Hi,

I'm developing a Minecraft Mod manager, and the goal for syncing the mods with the game is to download the latest version of the game (a 1.4MB .jar file) from the Minecraft site (http://minecraft.net/), every time the mods are synced.

I have created the following "SimpleDownloader" class, following the examples on the QT Documentation almost word-for-word to try and get this to work perfectly. I've already gone to several places including StackOverflow and have been Googling for days trying to fix this issue.

simpledownloader.h

@#ifndef SIMPLEDOWNLOADER_H
#define SIMPLEDOWNLOADER_H

#include <QObject>

#include <QNetworkAccessManager>
#include <QNetworkReply>
#include <QNetworkRequest>
#include <QFile>
#include <QStringList>
#include <QFileInfo>
#include <QDir>
#include <QUrl>
#include <QIODevice>
#include <QTime>

class SimpleDownloader : public QObject
{
Q_OBJECT

QNetworkAccessManager manager;
QUrl currentDownload;
QString currentSaveDirectory;
QTime *timer;

public:
SimpleDownloader();

void InitDownload(QString url, QString saveDirectory);

signals:
void Progress(int received, int total, QString message);
void Finished(QString filename, bool success, QString message);

private:
QString saveFileName(QUrl url);
bool saveReply(QString filename, QIODevice *data);

private slots:
void DownloadFinished(QNetworkReply *reply);
void DownloadProgress(qint64 bytesReceived, qint64 bytesTotal);
};

#endif // SIMPLEDOWNLOADER_H@

simpledownloader.cpp

@#include "simpledownloader.h"

SimpleDownloader::SimpleDownloader()
{
connect(&manager, SIGNAL(finished(QNetworkReply*)), this, SLOT(DownloadFinished(QNetworkReply*)));
}

void SimpleDownloader::InitDownload(QString url, QString saveDirectory) {
QUrl urlU(url);
QNetworkRequest request(urlU);
QNetworkReply *reply = manager.get(request);
qWarning("Request Started");
reply->connect(reply, SIGNAL(downloadProgress(qint64,qint64)), this, SLOT(DownloadProgress(qint64,qint64)));

this->currentDownload = url;
this->currentSaveDirectory = saveDirectory;

timer = new QTime();
timer->start();

}

void SimpleDownloader::DownloadFinished(QNetworkReply *reply) {
qWarning("Download Finished");
QUrl url = reply->url();
if (reply->error()) {
qCritical("Download Error.");
Finished("", false, "There was an error downloading the file.");
} else {
QString filename = saveFileName(url);
if (saveReply(filename, reply)) {
qWarning("Download succeeded.");
Finished(filename, true, "");
} else {
qWarning("Download failed.");
Finished(filename, false, "Could not write to the download file. Please make sure it exists.");
}
}
}

bool SimpleDownloader::saveReply(QString filename, QIODevice *data) {
QFile file(filename);
QFileInfo fileInformation(file);
QDir dir(fileInformation.dir());
dir.mkpath(dir.path());
if (file.open(QFile::WriteOnly)) {
file.write(data->readAll());
file.close();
return true;
} else
return false;
}

void SimpleDownloader::DownloadProgress(qint64 bytesReceived, qint64 bytesTotal) {
// Convert to integers.
int br = bytesReceived;
int bt = bytesTotal;

// Get speed. Borrowed from QT Documentation.
double speed = bytesReceived * 1000.0 / timer->elapsed();
QString unit;
if (speed < 1024) {
    unit = "bytes/sec";
} else if (speed < 1024*1024) {
    speed /= 1024;
    unit = "kB/s";
} else {
    speed /= 1024*1024;
    unit = "MB/s";
}

QString speedString = QString::number(speed);
Progress(br, bt, "Downloading at " + speedString + " " + unit);

}

QString SimpleDownloader::saveFileName(QUrl url) {
// Borrowed from the QT Examples

QString path = url.path();
QString basename = QFileInfo(path).fileName();

if (basename.isEmpty())
    basename = "download";

if (QFile::exists(basename)) {
    // already exists, don't overwrite
    int i = 0;
    basename += '.';
    while (QFile::exists(basename + QString::number(i)))
        ++i;

    basename += QString::number(i);
}

basename = basename.prepend(this->currentSaveDirectory + "/");

return basename;

}@

The Problem

The code compiles successfully, and runs. The problem is, that when InitDownload is called, it initializes the download and the qWarning message is displayed, but none of the signals are ever called, and the file never shows up where it is specified to be.

I'm assuming this is probably some random bit of code that I have typed wrong, or that there's something wrong with QNetworkAccessManager and the signals it calls... But I'm not sure.

Any suggestions?

Update

I'm currently downloading and installing the QT SDK 1.1 Release Candidate to see if there's some kind of fix that resolves this problem.

Update

After installing the new SDK, the problem still persists.

florent.revelut

Some quick comments:

your timer is never deleted (you lack the destructor)
check that you connection returns true. A good practice is to have an assert (it's removed in release) so you're sure that you didn't miss with case in signals / slots:
@
bool rc=connect (this,SIGNAL(totor),pObject,SLOT(onTotor))
Q_ASSERT(totor)
@
you only didn't connect the error signals : might be worth to help investigating what happens
line 72, you should use emit Progress(...) instead of calling the signal
check that your object is created from a context that has an event loop and that it isn't blocked (otherwise, your slot will never be called)

I don't see major flaws in this quick review, should work IMHO (I've similar code). If none of these helps, you can try having a network sniffer (wireshark) to try to see at network level what happens.

mario

Make sure that you keep the SimpleDownloader instance valid until you get the response (don't know if you already do it or not) else the NetworkManager will be deleted. I think that the request has the manager as parent which will also kill the request.

This happened to me when I was playing in Python and the garbage collector kicked in because I didn't hold a reference to the network manager.

florent.revelut

I did compile your code, and embed it into a QCoreApplication and it actually created a local file. Definitely, the issue is not in the portion of code you posted but on how you used it

main.cpp
@
#include <QCoreApplication>
#include "simpledownloader.h"

int main (int n, char** p) {
QCoreApplication a(n,p);
SimpleDownloader s;

s.InitDownload("http://10.0.1.77:8010/content/adaptive/bipbop/bipbopall.m3u8","./test");
a.exec();
} // main
@

simpledl.pro
@
TEMPLATE = app
TARGET = truc
DEPENDPATH += .
INCLUDEPATH += .

QT += network

Input

HEADERS += simpledownloader.h
SOURCES += main.cpp simpledownloader.cpp
@

I compiled it with visual 2008 / Qt 4.6.2 and ran it, it created a folder test and a file bipbop.m3u8 inside (my file is a very small one, 292 bytes)

Note that I did another test with a 80MB binary file and it worked too.

florent.revelut

Just as a reference, it's cross linked on "stackoverflow":http://stackoverflow.com/questions/5613168/qt-qnetworkaccessmanager-download-not-starting

sammarks

florent.revelut: Does it matter that I'm using a pointer instead of just the regular to initialize the class? Here's the code that I have to initialize, because I need it for the connect statements.

@SimpleDownloader *manager = new SimpleDownloader();
connect(manager, SIGNAL(Progress(int,int,QString)), this, SLOT(DownloadProgress(int,int,QString)));
connect(manager, SIGNAL(Finished(QString,bool,QString)), this, SLOT(DownloadComplete(QString,bool,QString)));
manager->InitDownload("http://minecraft.net/download/minecraft.jar", modDir + "/tempDL");@

Plus, in the class, there is a constructor that is necessary to connect the finished signal of the QNetworkAccessManager to the one in the class. I'm not sure if it's still called without using the new SimpleDownloader() statement.

After checking to see if all the connect statements were created successfully, they all return true.

And after adding the event loop, my code that initializes the class now looks like this:

@ SimpleDownloader *manager = new SimpleDownloader();
QEventLoop loop(this);
bool c1 = connect(manager, SIGNAL(Progress(int,int,QString)), this, SLOT(DownloadProgress(int,int,QString)));
bool c2 = connect(manager, SIGNAL(Finished(QString,bool,QString)), this, SLOT(DownloadComplete(QString,bool,QString)));
loop.exec();
qWarning("Connect: Progress: %d | Finished: %d", c1, c2);
//QUrl mcd("http://www.minecraft.net/download/minecraft.jar");
//manager->DownloadFileNew("http://minecraft.net/download/minecraft.jar", modDir + "/tempDL");
manager->InitDownload("http://minecraft.net/download/minecraft.jar", modDir + "/tempDL");
//DownloadComplete();@

And when run, I get the following error:

@QObject: Cannot create children for a parent that is in a different thread.
(Parent is SyncMods(0xbb80960), parent's thread is QThread(0x7f7728), current thread is SyncMods(0xbb80960)@

And just a helpful tidbit that's probably important, the parenting class that is calling the SimpleDownloader class is called SyncMods, which inherits QThread. It's supposed to be on a separate thread, but the SimpleDownloader class and the SyncMods class are supposed to be on the same thread seeing as the one calls the other. The SyncMods class is run asynchronously by the main thread. Not sure if that helps, but it's probably important.

Update

After reviewing the error, I discovered that it was the initializer for the QEventLoop. I have revised my code as follows:

@ SimpleDownloader *manager = new SimpleDownloader();
QEventLoop *loop = new QEventLoop();
bool c1 = connect(manager, SIGNAL(Progress(int,int,QString)), this, SLOT(DownloadProgress(int,int,QString)));
bool c2 = connect(manager, SIGNAL(Finished(QString,bool,QString)), this, SLOT(DownloadComplete(QString,bool,QString)));
loop->exec();
qWarning("Connect: Progress: %d | Finished: %d", c1, c2);
//QUrl mcd("http://www.minecraft.net/download/minecraft.jar");
//manager->DownloadFileNew("http://minecraft.net/download/minecraft.jar", modDir + "/tempDL");
manager->InitDownload("http://minecraft.net/download/minecraft.jar", modDir + "/tempDL");
//DownloadComplete();@

florent.revelut

Wow, you lost me...

I can only advise to :

check the origin of the Qt warning and read again and again about threading mechanism in Qt : each QObject lives in a QThread where it's created, that's where he'll receive signals.
check that your SyncMods class (derivating from thread) actually has an event loop : what are you doing in the run method of SyncMods ? Note that SyncMods itself is a QThread, thus a QObject and thus, living in another thread... It means that any slot of SyncMods will be called in its contrsutor thread.

goetz

QNetworkAccessManager is already asynchronous. There's hardly ever a need to put it into a thread.

If you really need to, read peppe's great wiki article on "Threads, Events and QObjects":http://developer.qt.nokia.com/wiki/Threads_Events_QObjects.

Regarding your question on "Does it matter that I’m using a pointer instead of just the regular to initialize the class?" Of course it does!

@
void foo() {
SimpleDownloader m;
m.start();
}
@

As the manager works asynchronously, start returns immediately. Then the method ends. Therefore m goes out of scope and is eventually destroyed, thus your download has no chance to finish.

If you use a pointer instead, the pointer goes out of scope, but the object it points still is alive and working. Although you have a memory leak now, since you do not have a valid pointer to the object anymore.

sammarks

Since the QNetworkAccessManager is already asynchronous (I thought it wasn't, which is why I was including it in the SyncMods class, thinking that it was like the .NET, which isn't asynchronous), I'm going to redo parts of my code to where the file is downloaded, and then the class is called.

I'll report back with my progress when I get a result.

Update

After fiddling around with the simpledownloader.cpp file and moving it to where the class was called from the main thread, I have come across a new issue. Whenever the program is executed, the download starts... and immediately finishes. I tried moving around what throws the finished() signal because I knew QNetworkReply had one, so I used that instead. Same issue. And, whenever I added a QEventLoop, all slots were blocked, meaning nothing happened. It simply stopped execution. Then again, I probably implemented QEventLoop wrong.

Here's the updated code - pay attention to the code that's commented out in case I commented out something that was useful:

simpledownloader.cpp

@#include "simpledownloader.h"

#include <QEventLoop>

SimpleDownloader::SimpleDownloader()
{
//QEventLoop loop = new QEventLoop();
//bool mc = connect(&manager, SIGNAL(finished(QNetworkReply)), this, SLOT(DownloadFinished(QNetworkReply*)));
//loop->exec();
//loop.deleteLater();
//qWarning("Manager Finished Connect: %d", mc);
}

void SimpleDownloader::InitDownload(QString url, QString saveDirectory) {
QUrl urlU(url);
QNetworkRequest request(urlU);
QNetworkReply *reply = manager.get(request);
qWarning("Request Started");
//QEventLoop *loop = new QEventLoop();
bool rc = reply->connect(reply, SIGNAL(downloadProgress(qint64,qint64)), this, SLOT(DownloadProgress(qint64,qint64)));
bool rc2 = reply->connect(reply, SIGNAL(error(QNetworkReply::NetworkError)), this, SLOT(DownloadError(QNetworkReply::NetworkError)));
//bool rc3 = reply->connect(reply, SIGNAL(finished()), this, SLOT(ReplyDownloadFinished()));
bool rc3 = reply->connect(reply, SIGNAL(readyRead()), this, SLOT(ReplyDownloadFinished()));
//loop->exec();
qWarning("Reply Progress Connect: %d | Reply error connect: %d | Reply Finished: %d", rc, rc2, rc3);

this->currentDownload = url;
this->currentSaveDirectory = saveDirectory;
this->reply = reply;

timer = new QTime();
timer->start();

}

void SimpleDownloader::DownloadFinished(QNetworkReply *reply) {
qWarning("Download Finished");
QUrl url = reply->url();
delete timer;
if (reply->error()) {
qCritical("Download Error.");
emit Finished("", false, "There was an error downloading the file.");
} else {
QString filename = saveFileName(url);
if (saveReply(filename, reply)) {
qWarning("Download succeeded.");
emit Finished(filename, true, "");
} else {
qWarning("Download failed.");
emit Finished(filename, false, "Could not write to the download file. Please make sure it exists.");
}
}
}

void SimpleDownloader::ReplyDownloadFinished() {
qWarning("Download Finished");
QUrl url = this->reply->url();
delete timer;
if (reply->error()) {
qCritical("Download Error.");
emit Finished("", false, "There was an error downloading the file.");
} else {
QString filename = saveFileName(url);
if (saveReply(filename, reply)) {
qWarning("Download succeeded.");
emit Finished(filename, true, "");
} else {
qWarning("Download failed.");
emit Finished(filename, false, "Could not write to the download file. Please make sure it exists.");
}
}
}

bool SimpleDownloader::saveReply(QString filename, QIODevice *data) {
QFile file(filename);
QFileInfo fileInformation(file);
QDir dir(fileInformation.dir());
dir.mkpath(dir.path());
if (file.open(QFile::WriteOnly)) {
file.write(data->readAll());
file.close();
return true;
} else
return false;
}

void SimpleDownloader::DownloadProgress(qint64 bytesReceived, qint64 bytesTotal) {
// Convert to integers.
int br = bytesReceived;
int bt = bytesTotal;

// Get speed. Borrowed from QT Documentation.
double speed = bytesReceived * 1000.0 / timer->elapsed();
QString unit;
if (speed < 1024) {
    unit = "bytes/sec";
} else if (speed < 1024*1024) {
    speed /= 1024;
    unit = "kB/s";
} else {
    speed /= 1024*1024;
    unit = "MB/s";
}

QString speedString = QString::number(speed);
emit Progress(br, bt, "Downloading at " + speedString + " " + unit);

}

QString SimpleDownloader::saveFileName(QUrl url) {
// Borrowed from the QT Examples

QString path = url.path();
QString basename = QFileInfo(path).fileName();

if (basename.isEmpty())
    basename = "download";

if (QFile::exists(basename)) {
    // already exists, don't overwrite
    int i = 0;
    basename += '.';
    while (QFile::exists(basename + QString::number(i)))
        ++i;

    basename += QString::number(i);
}

basename = basename.prepend(this->currentSaveDirectory + "/");

return basename;

}

void SimpleDownloader::DownloadError(QNetworkReply::NetworkError error) {
qCritical("Network Error");
//qCritical(error);
}@

The header file hasn't really changed, which is why I didn't add it to this post.

florent.revelut

Well, if you use the network access manager from the main thread, you don't have to think about event loop. I would say that 99% of the case, it's a bad idea to play with that unless you know what you're doing.
I strongly suggest you read

"peppe's excellent wiki page on threading":http://developer.qt.nokia.com/wiki/ThreadsEventsQObjects
"doc page about signals and slots":http://doc.qt.nokia.com/4.6/signalsandslots.html

Executive summary is :

most of the time, the event loop of the main thread, already here, is enough
before playing with threads, and especially qthread, be aware of all the multithread design patterns and common issues
if your signals / slots don't connect or are not received correctly, chances are that you're locking the context (inifinite loop or deadlock)

Moreover, currently, it's not clear what you're asking: download finishes instantly. Are you sure you check the error in the download manager (like bad proxy management, wrong url) ?

goetz

First: show us your main method, where you call your download manager. Your premature end of the download smells like a heap vs. stack allocation error or some event loop not running and the main function falling through to the end before time.

Then, even if everything runs correctly from a flow of control view, your save data is corrupted.

you do

@
connect(reply, SIGNAL(readyRead()), this, SLOT(ReplyDownloadFinished()));
@

Signal readyRead() is emitted as soon as data came in, the download progress might continue. In other words: you get readyRead before your download has finished. In ReplyDownloadFinished you save the available chunk to the file. But you do not append to an existing file, but override the previously stored data. So your file will only contain the last chunk received.

sammarks

Okay, thanks for your help so far.

First of all, I think it would help if I completely explained what I'm doing, because I discovered that instead of calling from the main thread, I might be calling from a different thread. I just read up some of the documentation for QDialog, and it led me to believe that it might be run on a different thread if it's not modal because it allows both windows to operate at the same time.

However, I have a special case for my dialog. I have the class JarProgress that inherits QDialog, which is set to be modal. There's a function in there called Sync, to be called after the class is initialized by the main thread.

I will include the SimpleDownloader class and header, the JarProgress class and header, and the function in the main class that calls the JarProgress Sync function. I have posted these on Pastebin because they're too large to post here.

"simpledownloader.cpp":http://pastebin.com/PciEmtH5
"simpledownloader.h":http://pastebin.com/L7T6Dj8H
"jarprogress.cpp":http://pastebin.com/tn09nBip
"jarprogress.h":http://pastebin.com/Ag8Hg2D3
"MineMod.cpp":http://pastebin.com/CdbtbzNX

I hope these files provide enough of an explanation as to what I might be doing wrong.

goetz

I have not looked into the code yet, just some short hints:

QDialog::exec() does not start a new thread, but it does start a new event loop. If your download manager runs in the main event loop, it may be stalled until the dialog is closed. This may be a situation where a new thread for the download manager could be a solution.

sammarks

[quote author="Volker" date="1302819398"]I have not looked into the code yet, just some short hints:

QDialog::exec() does not start a new thread, but it does start a new event loop. If your download manager runs in the main event loop, it may be stalled until the dialog is closed. This may be a situation where a new thread for the download manager could be a solution.[/quote]

The download manager is called from the QDialog, so that might not be a problem. However, when showing the dialog in the Sync() function, should I use .exec() instead of .show()? Or will that cease the execution of code until the dialog has closed?

sammarks

Problem Solved

I just though I'd let you all know why I had the problem, which is rather stupid if you think about it, haha.

The issue was, I was trying to download a file that was a HTTP 301 error, meaning it was a redirect. For some reason, the file wasn't downloading because of that. Now that I have found the correct link to the file, the download manager now works.

Thanks for all of your help, and for steering me in the correct direction.