Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NetworkTask: Distinguish implicit abort by timeout from explicit abort #10884

Closed
wants to merge 2 commits into from
Closed

NetworkTask: Distinguish implicit abort by timeout from explicit abort #10884

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
08cb847
WebTask: Document intermediate state change
uklotzde Sep 9, 2022
bf31e6a
NetworkTask: Distinguish implicit abort by timeout from explicit abort
uklotzde Sep 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/network/networktask.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,18 +51,19 @@ void NetworkTask::invokeAbort() {
QMetaObject::invokeMethod(
this,
#if QT_VERSION < QT_VERSION_CHECK(5, 10, 0)
"slotAbort"
"slotAbort",
Q_ARG(bool, false)
#else
[this] {
this->slotAbort();
this->slotAbort(false);
}
#endif
);
}

void NetworkTask::abort() {
DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
slotAbort();
slotAbort(false);
}

void NetworkTask::emitAborted(
Expand Down
6 changes: 5 additions & 1 deletion src/network/networktask.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,11 @@ class NetworkTask : public QObject {
int timeoutMillis = kNoTimeout,
int delayMillis = kNoStartDelay) = 0;
/// See also: invokeAbort()
virtual void slotAbort() = 0;
///
/// timedOut = false: The abort has been triggered explicitly.
/// timedOut = true: The abort has been triggered implicitly
/// after a client-side timeout expired.
virtual void slotAbort(bool timedOut) = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have a code path to deliver timeouts to the calling code. Introducing here an anonymous boolean to pass a timeout a different way is unnecessary and raises the code complexity unnecessarily. In addition it exposes implementation details to the calling code. It violates also the code symmetry, because in case of all other error cause the error message is created in the NetworkTask and its derived classes all other Errors are passed in a generic way including a translated message and detailed error codes-

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is bikeshedding. I already wanted to mention that I was too lazy to introduce an enum class before getting feedback on the proposed solution. I assumed that this is obvious.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have a code path to deliver timeouts to the calling code.

You still confuse client-side with server-side timeouts. A client-side timeout is an abort, but with an implicit instead of an explicit trigger. Technically, it is not an error, because the task did what you told it to do (according to the timeout parameter provided when starting it). A server-side timeout is an unexpected outcome and an error.

Aborting a request might subsequently let your business code fail. But that's a different level of abstraction.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is bikeshedding.

Seriously @uklotzde ? Please treat me with respect!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The client side timeout is an network error in the same way as the server side timeout. There is no need to pass one over a different signal than the other. In contrast the slotAbort is a direct responds to an explicit immediately abort request. Using that for a Network was wrong and causes the bug I have fixed.

But anyway this is an implementation detail f the bug fix.

You have not explained why you experience a bug with it.


signals:
/// The receiver is responsible for deleting the task in the
Expand Down
10 changes: 7 additions & 3 deletions src/network/webtask.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,11 +205,15 @@ void WebTask::slotStart(int timeoutMillis, int delayMillis) {

auto* const pNetworkAccessManager = m_networkAccessManagerWeakPtr.data();
VERIFY_OR_DEBUG_ASSERT(pNetworkAccessManager) {
// Internally switch the state to pending as a prerequisite before
// invoking onNetworkError(). This is only required for technical
// reasons and the intermediate state will never become visible.
m_state = State::Pending;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting the state to Pending here is just a hack to pass the debug assertion in onNetworkError(). The correct solution is to change the state to State::Failed after the previous call fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree.

onNetworkError(
QNetworkReply::NetworkSessionFailedError,
tr("No network access"),
WebResponseWithContent{});
DEBUG_ASSERT(m_state == State::Failed);
return;
}
DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(pNetworkAccessManager);
Expand Down Expand Up @@ -256,7 +260,7 @@ void WebTask::slotStart(int timeoutMillis, int delayMillis) {
Qt::UniqueConnection);
}

void WebTask::slotAbort() {
void WebTask::slotAbort(bool timedOut) {
DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
if (!isBusy()) {
DEBUG_ASSERT(m_timeoutTimerId == kInvalidTimerId);
Expand Down Expand Up @@ -312,7 +316,7 @@ void WebTask::slotAbort() {
// this scope.
const auto pendingNetworkReplyDeleter = ScopedDeleteLater(pPendingNetworkReply);
m_pendingNetworkReplyWeakPtr.clear();
doNetworkReplyAborted(pPendingNetworkReply);
doNetworkReplyAborted(pPendingNetworkReply, timedOut);
}

m_state = State::Aborted;
Expand Down Expand Up @@ -348,7 +352,7 @@ void WebTask::timerEvent(QTimerEvent* event) {
<< "Aborting after timed out";
// Trigger the regular abort workflow after a client-side
// timeout occurred
slotAbort();
slotAbort(true);
}

void WebTask::slotNetworkReplyFinished() {
Expand Down
10 changes: 8 additions & 2 deletions src/network/webtask.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ class WebTask : public NetworkTask {
void slotStart(
int timeoutMillis = kNoTimeout,
int delayMillis = kNoStartDelay) override;
void slotAbort() override;
void slotAbort(bool timedOut) override;

private slots:
void slotNetworkReplyFinished();
Expand Down Expand Up @@ -192,8 +192,14 @@ class WebTask : public NetworkTask {
int parentTimeoutMillis) = 0;

/// Optional: Do something after aborted.
///
/// The parameter `timedOut` is set to `true` if the abort has
/// been triggered implicitly by a client-side timeout.
///
/// See also: `NetworkTask::slotAbort()`
virtual void doNetworkReplyAborted(
QNetworkReply* abortedNetworkReply) {
QNetworkReply* abortedNetworkReply,
bool timedOut) {
Q_UNUSED(abortedNetworkReply);
}

Expand Down