11
\$\begingroup\$

Since Qt still does not support to set timeouts on QNetworkRequest objects, I wrote this little wrapper class:

/**
Usage:
new QReplyTimeout(yourReply, msec);

When the timeout is reached the given reply is closed if still running
*/
class QReplyTimeout : public QObject {
  Q_OBJECT
public:
  QReplyTimeout(QNetworkReply* reply, const int timeout) : QObject(reply) {
    Q_ASSERT(reply);
    if (reply) {
      QTimer::singleShot(timeout, this, SLOT(timeout()));
    }
  }

private slots:
  void timeout() {
    QNetworkReply* reply = static_cast<QNetworkReply*>(parent());
    if (reply->isRunning()) {
      reply->close();
    }
  }
};

You can use it in a very simple fire-and-forget manner:

QNetworkAccessManager networkAccessManger;
QNetworkReply* reply = networkAccessManger.get(QNetworkRequest(QUrl("https://www.google.com")));
new QReplyTimeout(r, 100);

If the call to Google does not finish in 100ms, it is aborted. And since the QReplyTimeout class is parented to the QNetworkReply, it will be destroyed automatically.

Review the code for any pitfalls, memory leaks, invalid casts and if it's generally in a good style.

\$\endgroup\$
2
  • \$\begingroup\$ class is quick and concise. I see no problems there. \$\endgroup\$ Commented Oct 5, 2015 at 11:13
  • 1
    \$\begingroup\$ I think this could only be used to shorten the timeout, not lengthen it. If you had a large file upload, I believe the internal Qt timeout would still trigger a QNetworkReply.TimeoutError \$\endgroup\$ Commented Jun 23, 2017 at 12:30

1 Answer 1

Reset to default
7
\$\begingroup\$

Here are my thoughts:

  1. It's a simple class that allocates quite a few times on the heap. You could minimize the number of implicit allocations it does. The QTimer::singleShot creates a temporary QObject instance and allocates a bunch on the heap. You can avoid it by handling a single-shot timer explicitly using timerEvent. You also avoid the need to set up any connections that way. A QObject does about two allocations when the first connection is added to it.

  2. Use Qt-5 style connections, but that doesn't apply anymore in light of #1 above.

  3. Add a static helper method that uses this class to set a timeout on a network request. The fact that you need to allocate ReplyTimeout is an implementation detail of sorts that could be abstracted out that way.

  4. Check if the reply is running before you set a timeout on it. Perhaps it was an incorrect/impossible request at the moment it was submitted to the manager and it was immediately finished.

  5. You're not supposed to name your classes with a Q prefix. It's reserved for Qt.

The code below is portable across Qt 4 and Qt 5:

class ReplyTimeout : public QObject {
  Q_OBJECT
  QBasicTimer m_timer;
public:
  ReplyTimeout(QNetworkReply* reply, const int timeout) : QObject(reply) {
    Q_ASSERT(reply);
    if (reply && reply->isRunning())
      m_timer.start(timeout, this);
  }
  static void set(QNetworkReply* reply, const int timeout) {
    new ReplyTimeout(reply, timeout);
  }
protected:
  void timerEvent(QTimerEvent * ev) {
    if (!m_timer.isActive() || ev->timerId() != m_timer.timerId())
      return;
    auto reply = static_cast<QNetworkReply*>(parent());
    if (reply->isRunning())
      reply->close();
    m_timer.stop();
  }
};

Use:

QNetworkAccessManager networkAccessManger;
QNetworkReply* reply = 
  networkAccessManger.get(QNetworkRequest(QUrl("https://www.google.com")));
ReplyTimeout::set(reply, 100);
\$\endgroup\$
4
  • 2
    \$\begingroup\$ This looks great, but if the upload/download is large won't this timeout even though it's transmitting? Should you cancel/restart the timer when uploadProgress is received? \$\endgroup\$ Commented Mar 30, 2017 at 19:39
  • \$\begingroup\$ @CaptRespect: I'd say that's outside the scope of responsibility of this class. It's the user's responsibility to specify a sensible timeout based on the knowledge about what exactly is being downloaded via the supplied QNetworkReply. \$\endgroup\$ Commented Jul 11, 2018 at 13:35
  • \$\begingroup\$ The Q_OBJECT macro is not required here and it hurts at least the compilation time (if not runtime weight of the object). Also, I would rename timeout to something like timeoutMsec so that it's immediately clear which units the timeout is in. \$\endgroup\$ Commented Jul 11, 2018 at 13:45
  • \$\begingroup\$ The Q_OBJECT macro is required on all QObject-derived classes. It is an implementation detail that it could be skipped on some classes, but it makes the classes thus "improved" not be QObjects in the Liskov Substitution Principle sense. E.g. all users of QObject expect that the metadata is correct and thus qobject_cast works. Skip Q_OBJECT and it's not true, thus you have a class that the compiler lets you use where a QObject can be used, but it's not really a QObject anymore. As for the timeout, it's Qt lingo for ms. If anything, I'd use std::duration instead. \$\endgroup\$ Commented Jul 13, 2018 at 11:16

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.