QNetworkAccessManager seems to leak with every retry

[Smaankers]

Hi,

I am implementing a client using QNetworkAccessManager. When the signal finished is received and the reply indicates an error, a timer is started and the reply marked for deletion. When the timer elapses, a retry is performed.

For testing purposes, I have set the retry timer to a very short amount of time (5 ms to shorten test time). To make sure there are no memory leaks when retrying, I am using a script on the commandline to watch the available memory: free -m | awk '/Mem:/{print $4}'

When I run the application with the network disconnected, I can see the free memory dropping about 1 megabyte per second. Even though retrying every 5 milliseconds is not what you would typically do, it does seem to demonstrate something is wrong.

My questions are:

• Am I doing something wrong in the code or in my testing?
• What can I do so fix either the code or the testing?

Hardware: Lenovo Thinkpad w/ Intel(R) Core(TM) i5-8250U
System: Linux Mint,
uname -a: Linux 4.10.0-38-generic #42~16.04.1-Ubuntu SMP Tue Oct 10 16:32:20 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
Qt version: 5.6.3
GCC: gcc (Ubuntu 5.4.0-6ubuntu1~16.04.10) 5.4.0 20160609
G++: g++ (Ubuntu 5.4.0-6ubuntu1~16.04.10) 5.4.0 20160609

Below is the code of the test. This version of the code was stripped down to make it easier to grasp, and it does show the behavior described above.

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

  ::testing::InitGoogleTest (&(argc), (argv));
  RUN_ALL_TESTS();
}

TEST(SmsNotifierStressTest, GIVEN_notifier_WHEN_notify_messages_THEN_successfully_sends_message)
{
    // initialization
    Sms_notifier notifier(true, 5);

    // state
    bool completion_handler_called = false;
    bool retry_handler_called = false;

    notifier.notify(
        "+0123456789",
          "Lorem Ipsum is simply dummy text of the printing and typesetting industry. ",
          [&completion_handler_called](Notification::SMS::Response_code){
              completion_handler_called = true;
          },
          [&retry_handler_called](){
              retry_handler_called = true;
          });

    qWarning() << "done, looping";

    while(true)
    {
      QCoreApplication::exec();
    }
}

And this is the code of the client:

QSemaphore Sms_notifier::queue_count(0);

Sms_notifier::Sms_notifier(bool test, int interval_length_milliseconds)

    :  QObject                       (NULL)
      ,m_test                        (test)
      ,m_interval_length_milliseconds(interval_length_milliseconds)
      ,m_manager                     ()
      ,m_timer                       ()
      ,m_addressee()
      ,m_payload()
      ,m_completion_handler()
      ,m_retry_handler()
{
  m_timer.setSingleShot(true);

  QObject::connect(&m_manager, &QNetworkAccessManager::finished,
                   this,       &Sms_notifier::on_nam_finished,
                   Qt::QueuedConnection);

  QObject::connect(&m_timer, &QTimer::timeout,
                   this,     &Sms_notifier::on_timer_elapsed,
                   Qt::QueuedConnection);

}

Sms_notifier::~Sms_notifier()
{
}

bool Sms_notifier::notify(
    const std::string addressee,
    const std::string payload,
    const std::function<void(Notification::SMS::Response_code)> completion_handler,
    const std::function<void()> retry_handler
)
{
    this->queue_count.release();

    m_retry_handler      = retry_handler;
    m_completion_handler = completion_handler;
    m_addressee          = addressee;
    m_payload            = payload;

    return notify();
}

int Sms_notifier::get_queue_count()
{
  return queue_count.available();
}

bool Sms_notifier::notify()
{
    // set up API query
    QUrlQuery params = prepare_request_parameters(m_addressee, m_payload, m_test);
    QUrl request_url(QString::fromStdString(Notification::SMS::SMS_API_URL));
    QNetworkRequest request(request_url);
    request.setHeader(QNetworkRequest::ContentTypeHeader, "application/x-www-form-urlencoded");

    // ssl configuration
    QSslConfiguration configuration = request.sslConfiguration();
    configuration.setProtocol(QSsl::TlsV1_2OrLater);
    request.setSslConfiguration(configuration);

    // make the API call
    this->m_manager.post(request, params.toString(QUrl::FullyEncoded).toUtf8());

    return false;
}

void Sms_notifier::on_nam_finished(QNetworkReply* reply)
{
    if (reply->error() != QNetworkReply::NetworkError::NoError)
    {
        if (m_retry_handler)
        {
            m_retry_handler();
        }

        m_timer.start(m_interval_length_milliseconds);
    }
    else
    {
        queue_count.acquire();

        // call completion handler with response code
        if (m_completion_handler)
        {
            int response_code_number = (QString(reply->readAll())).mid(0, 3).toInt();
            auto response_code       = static_cast<Notification::SMS::Response_code>(response_code_number);

            m_completion_handler(response_code);
        }

        m_completion_handler = nullptr;
        m_retry_handler      = nullptr;
        m_addressee.clear();
        m_payload.clear();
    }

    reply->deleteLater();
}

void Sms_notifier::on_timer_elapsed()
{
  notify();
}

QUrlQuery Sms_notifier::prepare_request_parameters(
    const std::string& addressee,
    const std::string& payload,
    bool test
)
{
    QUrlQuery params;

    params.addQueryItem("UID", "someid"));
    
    // ... some more params here

    return params;
}

[Pablo J. Rogina]

@Smaankers I guess you need to change

// make the API call
this->m_manager.post(request, params.toString(QUrl::FullyEncoded).toUtf8());

into

QNetworkReply *reply = this->m_manager.post(request, params.toString(QUrl::FullyEncoded).toUtf8());

[Smaankers]

@Pablo-J.-Rogina

Thank you for your answer.

I do not understand how saving the pointer and then not using it would change any behavior. As far as I can tell, the compiler would probably consider it as an unused variable and optimize it away.

Can you please explain what behavior you would expect to be different?

[VRonin]

@Smaankers said in QNetworkAccessManager seems to leak with every retry:

Am I doing something wrong in the code or in my testing?

Is this all your code?
Are you using multithreading?
Are you blocking the event loop somewhere else in your code?

[Pablo J. Rogina]

@Smaankers said in QNetworkAccessManager seems to leak with every retry:

Can you please explain what behavior you would expect to be different?

I expect that Qt framework can handle the pointer/memory management right now that there's a pointer assigned and it is not lost as you did previously.

[JonB]

@Pablo-J.-Rogina

this->m_manager.post(request, params.toString(QUrl::FullyEncoded).toUtf8());

versus

QNetworkReply *reply = this->m_manager.post(request, params.toString(QUrl::FullyEncoded).toUtf8());

Changing the line in the OP's code to assign to a (pointer) local variable, which is then not used, will not trigger anything in the Qt framework. It's just as "lost" with that code as the original --- it's purely a compiler issue. Unless you mean the OP to use that variable, or keep it somewhere and later use it.

P.S.
Though if I were @Smaankers I'd spend 5 seconds to try it, just in case... :) I don't know if compilers will warn on unused variable....

[Smaankers]

@VRonin

"Is this all your code?"
Yes, this is all the code. I stripped it down to the minimum to make sure there was nothing else causing the leak.

"Are you using multithreading?"
No, there is just the main thread that you see in the code.

"Are you blocking the event loop somewhere else in your code?"
No, at the end of the test it executes the event loop without any interruptions.

[Smaankers]

@JonB

"Changing the line in the OP's code to assign to a (pointer) local variable, which is then not used, will not trigger anything in the Qt framework."

That was exactly my thinking.

"Though if I were @Smaankers I'd spend 5 seconds to try it, just in case... :)"

I did, but there is no change in behavior.

[JonB]

@Smaankers
:)

Now, I'm not a Qt C++-er, I'm a PyQt Pythonista, so (given its auto reference counting) I don't get your memory leaks or have to think about freeing objects....

Following on from the theme of @Pablo-J-Rogina's post/concern/suggestion:

• Each time round your timer you set off a fresh notify(); call.

• That does a new this->m_manager.post(request, params.toString(QUrl::FullyEncoded).toUtf8());.

• http://doc.qt.io/qt-5/qnetworkaccessmanager.html#post says

Sends an HTTP POST request to the destination specified by request and returns a new QNetworkReply object opened for reading that will contain the reply sent by the server.

• Note the word "new". I'm thinking/wondering whether that doesn't mean you are responsible for freeing that object, or what do you think will do so for you?

• You would free your reply object in on_nam_finished, but that's where you do your retry, and start off the timer again....

I'm getting a bit lost, but are you thereby leaking a QNetworkReply object each time? Can you verify that for each post() made the corresponding QNetworkReply returned is indeed deleted?

[Smaankers]

Okay I found the culprit in an unexpected corner.

I decided to completely strip it to the absolute bare minimum:

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

  // initialization
  Sms_notifier notifier(true, 5);

  notifier.notify(
      "+0123456789",
      "Lorem Ipsum is simply dummy text of the printing and typesetting industry. ");

  qWarning() << "done, looping";

  while(true)
  {
    QCoreApplication::exec();
  }

}

Sms_notifier::Sms_notifier(bool test, int interval_length_milliseconds) 
 
    :  QObject                       (NULL) 
      ,m_test                        (test) 
      ,m_interval_length_milliseconds(interval_length_milliseconds) 
      ,m_manager                     () 
      ,m_timer                       () 
      ,m_addressee() 
      ,m_payload() 
{ 
  m_timer.setSingleShot(true); 
 
  QObject::connect(&m_manager, &QNetworkAccessManager::finished, 
                   this,       &Sms_notifier::on_nam_finished); 
 
  QObject::connect(&m_timer, &QTimer::timeout, 
                   this,     &Sms_notifier::on_timer_elapsed); 
 
} 
 
Sms_notifier::~Sms_notifier() 
{ 
} 
 
bool Sms_notifier::notify( 
    const std::string addressee, 
    const std::string payload 
) 
{ 
    m_addressee = addressee; 
    m_payload   = payload; 
 
    return notify(); 
} 
 
bool Sms_notifier::notify() 
{ 
    QNetworkRequest request; 
    QByteArray data; 

    m_manager.post(request, data); 
    return false; 
} 
 
void Sms_notifier::on_nam_finished(QNetworkReply* reply) 
{ 
    QNetworkReply::NetworkError error = reply->error(); 
    reply->deleteLater(); 
 
    if (error != QNetworkReply::NetworkError::NoError) 
    { 
        m_timer.start(m_interval_length_milliseconds); 
    } 
    else 
    { 
        qWarning() << "success"; 
 
        m_addressee.clear(); 
        m_payload.clear(); 
    } 
} 
 
void Sms_notifier::on_timer_elapsed() 
{ 
  notify(); 
} 

It turns out it was still leaking while there was no network. So I stripped away all libraries that were linked, and it still leaked.

Eventually my eye struck this in the .pro file:

QMAKE_CXXFLAGS += -fsanitize=address
QMAKE_CFLAGS   += -fsanitize=address
QMAKE_LFLAGS   += -fsanitize=address -lasan

This was added to detect memory leaks and report them when the application quits. After removing this, the excessive "memory leak" was gone.

I am assuming that the address sanitizer allocates memory for each allocation done by the application, for its own administration purposes. I suspect that when the application releases the allocated memory, the sanitizer holds on to the respective administration data until the application quits. This could explain why when I remove the sanitizer it also removes the leak.

Well, thanks everyone for your input!