Qt Network seems to persistently use memory

[LorenDB]

@JonB @jeremy_k OK, point taken. I will switch back to QEventLoop. However, the memory usage problem will still persist with QEventLoop, if memory serves me correctly (no pun intended).

[jeremy_k]

Can you post a complete, minimal working example of why dropping back to the original event loop won't work? So far, I've seen nothing to support the claim but the claim itself.

[LorenDB]

I'd be glad to! I'll put something together either tomorrow or Monday.

[jeremy_k]

@LorenDB You might also want to try removing or simulating the network communication to see if the leak goes away.

[LorenDB]

Thanks for reminding me: when I wrote a test app that never actually used the internet connection, there was no memory creep (although for some reason the memory jumped and then went back down every 5 seconds).

[LorenDB]

All right! As requested, here is a minimal working example that is basically a mashup of various code from my application, plus a bit of demonstration in main():

#include <iostream>

#include <QCoreApplication>
#include <QNetworkAccessManager>
#include <QNetworkReply>
#include <QUrlQuery>
#include <QTimer>

class ClockifyUser : public QObject
{
public:
	explicit ClockifyUser(QByteArray apiKey, QString userId, QObject *parent = nullptr)
		: QObject{parent},
		  m_apiKey{apiKey},
		  m_userId{userId}
	{
		m_manager.setAutoDeleteReplies(true);
	}

	bool hasRunningTimeEntry()
	{
		QUrl url{"https://api.clockify.me/api/v1/workspaces/<workspace id goes here; fill it in yourself>/user/" + m_userId + "/time-entries"};
		QUrlQuery query;
		query.addQueryItem("in-progress", "true");
		url.setQuery(query);

		bool status = false;

		get(url, false, 200, [&status](QNetworkReply *rep) {
			if (auto text = rep->readAll(); !text.isEmpty() && text != "[]")
				status = true;
		}, [](QNetworkReply *) { /* we won't do anything on error */ });

		return status;
	}

private:
	void get(const QUrl &url,
			 bool async,
			 int expectedReturnCode,
			 const std::function<void (QNetworkReply *)> &successCb,
			 const std::function<void (QNetworkReply *)> &failureCb)
	{
		QNetworkRequest req{url};
		req.setHeader(QNetworkRequest::ContentTypeHeader, "application/json");
		req.setRawHeader("X-Api-Key", m_apiKey);

		auto rep = m_manager.get(req);
		m_pendingReplies.insert(rep, {successCb, failureCb});

		// Please do not question my sanity here; I actually am pretty sure that I know what I am doing here
		bool *done = async ? nullptr : new bool{false};

		connect(rep, &QNetworkReply::finished, this, [this, rep, expectedReturnCode, done]() {
			if (auto status = rep->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt(); status == expectedReturnCode) [[likely]]
					m_pendingReplies[rep].first(rep);
			else [[unlikely]]
					m_pendingReplies[rep].second(rep);

			if (done != nullptr)
				*done = true;

			m_pendingReplies.remove(rep);
		}, Qt::DirectConnection);

		if (!async) // OK, I know, I haven't made this into a QEventLoop yet
			while (!(*done))
			{
				qApp->processEvents();
				qApp->sendPostedEvents();
			}

		if (done)
		{
			delete done;
			done = nullptr; // in case we are doing async, this is required to make the lamba's done setting logic work
		}
	}
	
	QHash<QNetworkReply *, QPair<std::function<void (QNetworkReply *)>, std::function<void (QNetworkReply *)>>> m_pendingReplies;
	
	QString m_userId;
	QByteArray m_apiKey;
	
	QNetworkAccessManager m_manager;
};

int main(int argc, char *argv[])
{
	QCoreApplication app{argc, argv};

	std::cout << "starting...\n";
	
	ClockifyUser user{"your api key goes here", "your user id goes here"};
	
	auto someRandomFunctionThatExecutesInTheEventLoopOrSomewhereLikeThat = [&user] {
		// Right here, I temporarily halt the code that provides updated statuses to the user interface. While it may
		// seem odd, it actually prevents stuttering for reasons that I won't go into now
		if (user.hasRunningTimeEntry())
		{
			// take some action here, like...
			std::cout << "Good for you, you seem to be working!" << std::endl; // Trivia: std::cout doesn't flush automatically when used in the Qt event loop
			// ... or something like that
		}
		else
			std::cout << "Why are you not working?" << std::endl;

		// now we can fire the status update stuff back up
	};

	QTimer timer;
	timer.setInterval(5000);
	timer.setSingleShot(false);
	timer.callOnTimeout(someRandomFunctionThatExecutesInTheEventLoopOrSomewhereLikeThat);

	std::cout << "timer starting...\n";
	timer.start();

	// now watch memory usage with your favorite memory usage visualization tool (Visual Studio debugger, Heaptrack, Valgrind, etc. etc.) and see it go up!
	
	return app.exec();
}

Please take note of several things:

• If you actually want to run this code, you'll have to modify the API key, user ID, and workspace ID that I redacted.
• This is a small sample of what my actual codebase looks like; there are other functions that return values from network requests as well.

Theoretically, this could be refactored into an elaborate callback system, but I'm not sure that it would be a good idea to do so, since it would result in complex callback chains 3 or 4 deep. For example:

connect(this, &SomeClass::someAction, &user, [&user] {
    user.fetchHasRunningTimeEntry();
    connect(&user, &ClockifyUser::hasRunningTimeEntryLoaded, &user, [&user](bool status) {
        if (status)
        {
            // ... execute a stop action, then a start action based on a return value from the stop action ...
            user.stopRunningTimeEntry();
            connect(&user, &ClockifyUser::runningTimeEntryStopped, &user, [&user] {
                user.startRunningTimeEntry(/* whatever params */);
            });
        }
        else
            user.startRunningTimeEntry(/* whatever params */); // ... you get the point ...
    });
});

Hope this helps clear up why I am doing what I am doing.

[Edit: I fixed the example callback-chain code to be a realistic example of what the callback setup would look like, since I realized that my original code wasn't very... correct]

[jeremy_k]

@LorenDB said in Qt Network seems to persistently use memory:

All right! As requested, here is a minimal working example that is basically a mashup of various code from my application, plus a bit of demonstration in main():

Thanks. In particular, thanks for realizing that a separate header file isn't necessary. I would have cut all of the parameters related to this particular service, and used a dummy URL to demonstrate, but that's minor.

	void get(const QUrl &url,
			 bool async,
			 int expectedReturnCode,
			 const std::function<void (QNetworkReply *)> &successCb,
			 const std::function<void (QNetworkReply *)> &failureCb)
	{
		QNetworkRequest req{url};
		req.setHeader(QNetworkRequest::ContentTypeHeader, "application/json");
		req.setRawHeader("X-Api-Key", m_apiKey);

		auto rep = m_manager.get(req);
		m_pendingReplies.insert(rep, {successCb, failureCb});

		// Please do not question my sanity here; I actually am pretty sure that I know what I am doing here
		bool *done = async ? nullptr : new bool{false};

!!!!? There's knowing what it's doing today, and there's you or someone else understanding in a year.
The async variable remains in scope to test later in the function. Using dynamic allocation to create a tristate variable seems like unnecessary complexity, for the sake of reducing performance. If you really like the tristate, create an enum.

  connect(rep, &QNetworkReply::finished, this, [this, rep, expectedReturnCode, done]() {
  	if (auto status = rep->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt(); status == expectedReturnCode) [[likely]]
  			m_pendingReplies[rep].first(rep);
  	else [[unlikely]]
  			m_pendingReplies[rep].second(rep);

  	if (done != nullptr)
  		*done = true;

  	m_pendingReplies.remove(rep);
  }, Qt::DirectConnection);

I'm not a fan of lambda functions that do much more than reorder function parameters, or capture one for later use. They seem to be hard to read and hard to test in comparison to a named function. Much like the pointer to a dynamically allocated bool, working should not be the only goal.

  if (!async) // OK, I know, I haven't made this into a QEventLoop yet
  	while (!(*done))
  	{
  		qApp->processEvents();
  		qApp->sendPostedEvents();
  	}

I wouldn't even do that. Return to whatever event loop led to this function call, and remove a little complexity from the program maintainer's life.

  if (done)
  {
  	delete done;
  	done = nullptr; // in case we are doing async, this is required to make the lamba's done setting logic work

Delete on a nullptr is a no-op. There's no need to check first.
The lambda above captures done as a copy. It can't see the nullptr assignment. Also, if async is true, done was already assigned nullptr, and the lambda only touches it if it isn't null.

int main(int argc, char *argv[])
{
QCoreApplication app{argc, argv};

std::cout << "starting...\n";

ClockifyUser user{"your api key goes here", "your user id goes here"};

auto someRandomFunctionThatExecutesInTheEventLoopOrSomewhereLikeThat = [&user] {
// Right here, I temporarily halt the code that provides updated statuses to the user interface. While it may
// seem odd, it actually prevents stuttering for reasons that I won't go into now

I'm not sure what "the code that provides updated statuses" means. That could be disconnecting a connection that updates a label, which sounds reasonable, or it could be another problem to solve rather than work around.

  if (user.hasRunningTimeEntry())
  {
  	// take some action here, like...
  	std::cout << "Good for you, you seem to be working!" << std::endl; // Trivia: std::cout doesn't flush automatically when used in the Qt event loop
  	// ... or something like that
  }
  else
  	std::cout << "Why are you not working?" << std::endl;

  // now we can fire the status update stuff back up

};

QTimer timer;
timer.setInterval(5000);
timer.setSingleShot(false);
timer.callOnTimeout(someRandomFunctionThatExecutesInTheEventLoopOrSomewhereLikeThat);

std::cout << "timer starting...\n";
timer.start();

// now watch memory usage with your favorite memory usage visualization tool (Visual Studio debugger, Heaptrack, Valgrind, etc. etc.) and see it go up!

return app.exec();
}

• This is a small sample of what my actual codebase looks like; there are other functions that return values from network requests as well.

Theoretically, this could be refactored into an elaborate callback system, but I'm not sure that it would be a good idea to do so, since it would result in complex callback chains 3 or 4 deep.

Signals and slots are an elaborate callback system. Choosing to use Qt and avoid them seems like a strange choice.

For example:

connect(this, &SomeClass::someAction, &user, [&user] {
    user.fetchHasRunningTimeEntry();
    connect(&user, &ClockifyUser::hasRunningTimeEntryLoaded, &user, [&user](bool status) {
        if (status)
        {
            // ... execute a stop action, then a start action based on a return value from the stop action ...
            user.stopRunningTimeEntry();
            connect(&user, &ClockifyUser::runningTimeEntryStopped, &user, [&user] {
                user.startRunningTimeEntry(/* whatever params */);
            });
        }
        else
            user.startRunningTimeEntry(/* whatever params */); // ... you get the point ...
    });
});

Writing small functions that do one thing may lead to needing a few more functions, but it will also likely be easier to understand each function. It also makes using a debugger easier because there's no need to differentiate execution from declaration.

Hope this helps clear up why I am doing what I am doing.

What, yes. Why, ...
The code looks like a difficult read. As pointed out, it already has behavioral issues and some unusual conventions. I don't see anything that couldn't be refactored to use a flatter structure and bear more resemblance to the Qt examples and documentation. Going over heavily trodden ground means it's more likely that the library code will work as intended.

[Edit: I fixed the example callback-chain code to be a realistic example of what the callback setup would look like, since I realized that my original code wasn't very... correct]

I hope you were at least half as lost as I am :-)
If refactoring isn't an option, good luck.

[LorenDB]

Wow, what a detailed reply! Before I dig into it, I'd like to point out that while I appreciate any suggestions for improvement, I'm not sure that all of this digging into how I have my HTTP calls structured is actually helping with my original problem (although, to be fair, it may be at least part of the problem). (Again, I am happy for the code criticism; I'm just saying that this seems to have turned into a bit of a code review.)

I would have cut all of the parameters related to this particular service, and used a dummy URL to demonstrate, but that's minor.

I actually thought about that, but decided that giving an example using the actual service would perhaps show better what I'm doing.

!!!!? There's knowing what it's doing today, and there's you or someone else understanding in a year.

That's a good point, and one that I probably don't think of enough.

The async variable remains in scope to test later in the function. Using dynamic allocation to create a tristate variable seems like unnecessary complexity, for the sake of reducing performance. If you really like the tristate, create an enum.

Hmm... an enum is a good thought. My idea here was something like "if done is nullptr, that means to ignore it and continue asynchronously. Otherwise, wait for it to be true before proceeding." The tristate setup was supposed to prevent some sort of bad behavior, but it actually might be a legacy holdover.

I'm not a fan of lambda functions that do much more than reorder function parameters, or capture one for later use. They seem to be hard to read and hard to test in comparison to a named function. Much like the pointer to a dynamically allocated bool, working should not be the only goal.

I see what you mean; although I do like using lambdas wherever it makes sense, this could be a location that I could remove the lambda due to the fact that I have the exact same thing elsewhere (I know, it's terrible).

Return to whatever event loop led to this function call, and remove a little complexity from the program maintainer's life.

Again, I hesitate to do that simply because I'm looking for a good method of getting a value from the network, and the most logical way from my perspective is getting it synchronously.

One thought I have on this is that maybe I should try using QCoro, in which case I could probably just co_await the network request. Unfortunately, I'm not sure how well setting up QCoro and coroutines would go in this project. Oh well...

Delete on a nullptr is a no-op. There's no need to check first.

I never knew that, interesting.

The lambda above captures done as a copy. It can't see the nullptr assignment.

Oops! I guess when I captured by copy, I thought that I was getting the underlying value anyway, so copy was fine. Evidently not.

Also, if async is true, done was already assigned nullptr, and the lambda only touches it if it isn't null.

I forget what "logic" was going through my head when I wrote that, although I'm sure I had a reason.

I'm not sure what "the code that provides updated statuses" means. That could be disconnecting a connection that updates a label, which sounds reasonable, or it could be another problem to solve rather than work around.

It's actually updating some properties on QSystemTrayIcons, but yeah, you seem to be getting the point.

Signals and slots are an elaborate callback system. Choosing to use Qt and avoid them seems like a strange choice.

I understand, although as I have said multiple times, my desire for a synchronous solution is what led me to try to avoid them in this context.

Writing small functions that do one thing may lead to needing a few more functions, but it will also likely be easier to understand each function. It also makes using a debugger easier because there's no need to differentiate execution from declaration.

Not sure that I understand this exactly, but I think I may get the overall point of what you are trying to say here.

What, yes. Why, ...
The code looks like a difficult read. As pointed out, it already has behavioral issues and some unusual conventions. I don't see anything that couldn't be refactored to use a flatter structure and bear more resemblance to the Qt examples and documentation. Going over heavily trodden ground means it's more likely that the library code will work as intended.

If you have any suggestions for how to implement a more Qt-ish solution that prevents network calls from executing out-of-order from each other and that lets me use a reasonable variable access system (e.g. the user.hasRunningTimeEntry() function that I posted), I'd be glad to look at them.

If refactoring isn't an option, good luck.

Is refactoring an option? Yes. Is it a good option? Not necessarily. If I knew that it would fix my memory problem, I'd refactor the whole program in a heartbeat. However, the sheer amount of work that refactoring will require makes me wonder if it's the best option.

I'd like to show you the full code, but I'll need to check with my employer before publicly sharing all the code. Anyway, I do appreciate the input.

[jeremy_k]

@LorenDB said in Qt Network seems to persistently use memory:

If you have any suggestions for how to implement a more Qt-ish solution that prevents network calls from executing out-of-order from each other and that lets me use a reasonable variable access system (e.g. the user.hasRunningTimeEntry() function that I posted), I'd be glad to look at them.

Fair enough. I've handed out enough criticism for a while. This hasn't been tested, won't compile, and is an oversimplification, but:

class StateManager : public QObject {
    Q_OBJECT
public:
    StateManager(QObject *parent = nullptr) : QObject{parent}, m_state{State::Start}
    {
        connect(&m_netManager, &QNetworkAccessManager::finished, this, &updateState);
        m_netManager.setAutoDeleteReplies(true);
    }

    void startSequence()
     {
        if (State::Start != m_state) {
            qDebug() << "You already started!";
        }
        else {
            m_netManager.get("https://someurl/start");
            m_state = State::First;
        }
     }
signals:
    void done();

slots:
    void updateState(QNetworkReply *reply)
    {
       if (reply.error() != QNetworkReply::NoError) {
           qDebug() << "An error occurred. Giving up";
           m_state = State::Start;
        }
        else {
           switch(m_state) {
           case State::First:
                m_netManager.get(QString("https://someurl/%1").arg(reply.readAll());
                m_state = State::Second;
                break;
           case State::Second:
                qDebug() << "The answer is" << QString::fromUtf8(reply.readAll());
                m_state = State::Start;
                break;
           default:
                qDebug() << "This isn't a valid state transition";
           }
        }
        if (State::Start == m_state)
            emit done();
    }

    enum class State { Start, First, Second } m_state;
    QNetworkAccessManager m_netManager;
};

• Connect to the done signal to schedule synchronous-like processing, and disable, deschedule, or disconnect as necessary until then.
• Use an external QNetworkAccessManager if concurrent sessions with multiple servers is required.
• Add a stateUpdated() signal if some things need intermediate updates.
• Store the QNetworkReply pointers if mid-sequence cancellation is needed.
• Move the logic in each state transition into separate functions if the state machine grows too large.

I'd like to show you the full code, but I'll need to check with my employer before publicly sharing all the code. Anyway, I do appreciate the input.

This isn't a great medium for large code samples. Github, Gitlab, etc are better for sharing large code samples if you think other people will be interested.

[LorenDB]

• Connect to the done signal to schedule synchronous-like processing, and disable, deschedule, or disconnect as necessary until then.

I still have my doubts about how well this method will work in my code, since it's not truly synchronous, but I guess that I can give it a try. However, I wrote a test app that downloaded a large file every file seconds that looks like this:

#include <iostream>

#include <QCoreApplication>
#include <QNetworkAccessManager>
#include <QNetworkReply>
#include <QUrlQuery>
#include <QTimer>

class NetworkSyncGetter : public QObject
{
public:
    explicit NetworkSyncGetter(QObject *parent = nullptr)
    : QObject{parent}
    {
        m_manager.setAutoDeleteReplies(true);
    }

    void get(const QUrl &url,
             const std::function<void (QNetworkReply *)> &successCb,
             const std::function<void (QNetworkReply *)> &failureCb)
    {
        QNetworkRequest req{url};

        auto rep = m_manager.get(req);
        m_pendingReplies.insert(rep, {successCb, failureCb});

        // since this example removes all async behavior, I was able to simplify this a bit
        bool done{false};

        connect(rep, &QNetworkReply::finished, this, [this, rep, &done]() {
            if (auto status = rep->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt(); status == 200) [[likely]]
                m_pendingReplies[rep].first(rep);
            else [[unlikely]]
                m_pendingReplies[rep].second(rep);

            done = true;

            m_pendingReplies.remove(rep);
        }, Qt::DirectConnection);

        while (!done) // still no QEventLoop
        {
            qApp->processEvents();
            qApp->sendPostedEvents();
        }
    }

private:
    QHash<QNetworkReply *, QPair<std::function<void (QNetworkReply *)>, std::function<void (QNetworkReply *)>>> m_pendingReplies;

    QNetworkAccessManager m_manager;
};

int main(int argc, char *argv[])
{
    QCoreApplication app{argc, argv};

    NetworkSyncGetter getter;

    QTimer timer;
    timer.setInterval(5000);
    timer.setSingleShot(false);
    timer.callOnTimeout([&] {
        QString s;

        getter.get(QUrl{"https://sabnzbd.org/tests/internetspeed/20MB.bin"}, [&](QNetworkReply *rep) {
            s = rep->readAll();
        }, [](QNetworkReply *rep) {});
    });
    timer.start();

    return app.exec();
}

(Yes, I know that the lambdas in this code go against everything that you said. Forgive me.)

That app does not experience the memory creep; therefore, I don't think that my method of synchronous requests is to blame. Admittedly, the code in this class differs somewhat from what my actual code looks like, but it's similar enough that I doubt that I made any game-changing modifications (other than the fact that I now don't use a dynamically-allocated bool).

• Use an external QNetworkAccessManager if concurrent sessions with multiple servers is required.

I have thought before that this might be a good idea. In this application, I won't need to use an external QNetworkAccessManager but I may need to in another project.

• Add a stateUpdated() signal if some things need intermediate updates.
• Store the QNetworkReply pointers if mid-sequence cancellation is needed.

I probably don't need this sort of stuff.

• Move the logic in each state transition into separate functions if the state machine grows too large.

Yeah, it's never good to have anything too large and bloated.

This isn't a great medium for large code samples. Github, Gitlab, etc are better for sharing large code samples if you think other people will be interested.

Don't worry, I had and have absolutely no intentions of uploading a project of this size to the forum directly.

[LorenDB]

For interest's sake, here is a link to the full project: https://github.com/ChristianLightServices/ClockifyTrayIcons. I did some further testing that left me not-so-convinced that my network accessing method is the problem; however, it could still be.