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

connection: emit networkError in all cases #683

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

connection: emit networkError in all cases #683

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 6 additions & 9 deletions Quotient/connection.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,10 +211,7 @@ void Connection::assumeIdentity(const QString& mxId, const QString& accessToken)
d->completeSetup(job->userId());
});
connect(job, &BaseJob::failure, this, [this, job] {
if (job->error() == BaseJob::StatusCode::NetworkError)
emit networkError(job->errorString(), job->rawDataSample(),
job->maxRetries(), -1);
else
if (job->error() != BaseJob::StatusCode::NetworkError)
Copy link
Member

Choose a reason for hiding this comment

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

You're effectively reverting e9c3d37 here, which reopens #614. networkError() is emitted here with -1 for the time until the next attempt to signify that the job has run out of attempts (probably it should be documented). Quaternion relies on this to retry assumeIdentity() indefinitely; but now that I think of it, it's probably reasonable to just make assumeIdentity() try indefinitely instead, the same way SyncJob does. In any case, that piece has to be either reverted, or overhauled.

Copy link
Author

@tdfischer tdfischer Jul 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

When you mean "that piece", are you referring to the magic -1 handling? Would you suggest modifying assumeIdentity() to have that behavior should happen here in quotient or in quaternon? Or would it be better done by just adding setMaxRetries(std::numeric_limits<int>::max()); to GetTokenOwnerJob?

Copy link
Member

@KitsuneRal KitsuneRal Jul 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I mean the way assumeIdentity() deals with network errors. I think it would be ideal to call setMaxRetries() on this particular job instance.

GetTokenOwnerJob itself is generated from the API definition, you can't change it directly - and actually, you shouldn't because it would affect all respective requests, including those from the client code. As of now, setMaxRetries() is protected but there's actually no particular reason it should be this way (it's actually public, so point 1 below is done). So what I suggest is:

  1. Make BaseJob::setMaxRetries() public. As far as I can tell, it doesn't even break the ABI (and definitely doesn't break the API) compatibility.
  2. Call job->setMaxRetries() on the job object returned by callApi(). This is safe to do at any point in the job's lifecycle, no need to delay the job execution.
  3. Delete emission of networkError() (you already did it here).

emit loginError(job->errorString(), job->rawDataSample());
});
});
Expand Down Expand Up @@ -448,11 +445,6 @@ void Connection::sync(int timeout)
d->syncJob = nullptr;
emit syncDone();
});
connect(job, &SyncJob::retryScheduled, this,
[this, job](int retriesTaken, int nextInMilliseconds) {
emit networkError(job->errorString(), job->rawDataSample(),
retriesTaken, nextInMilliseconds);
});
connect(job, &SyncJob::failure, this, [this, job] {
// SyncJob persists with retries on transient errors; if it fails,
// there's likely something serious enough to stop the loop.
Expand Down Expand Up @@ -1624,6 +1616,11 @@ BaseJob* Connection::run(BaseJob* job, RunningPolicy runningPolicy)
// Reparent to protect from #397, #398 and to prevent BaseJob* from being
// garbage-collected if made by or returned to QML/JavaScript.
job->setParent(this);
connect(job, &BaseJob::retryScheduled, this,
[this, job](int retriesTaken, int nextInMilliseconds) {
emit networkError(job->errorString(), job->rawDataSample(),
retriesTaken, nextInMilliseconds);
});
connect(job, &BaseJob::failure, this, &Connection::requestFailed);
job->initiate(d->data.get(), runningPolicy & BackgroundRequest);
return job;
Expand Down
4 changes: 2 additions & 2 deletions Quotient/connection.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -766,12 +766,12 @@ public Q_SLOTS:
void stateChanged();
void loginError(QString message, QString details);

//! \brief A network request (job) started by callApi() has failed
//! \brief A network request (job) started by run() (including callApi()) has failed
//! \param request the pointer to the failed job
//! \sa callApi
void requestFailed(Quotient::BaseJob* request);

//! \brief A network request (job) failed due to network problems
//! \brief A network request (job) started by run() (including callAPI()) failed due to network problems
//!
//! This is _only_ emitted when the job will retry on its own;
//! once it gives up, requestFailed() will be emitted.
Expand Down
Binary file added Quotient/csapi/.whoami.cpp.swp
View file
Open in desktop
Binary file not shown.
1 change: 1 addition & 0 deletions Quotient/csapi/whoami.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,5 @@ GetTokenOwnerJob::GetTokenOwnerJob()
makePath("/_matrix/client/v3", "/account/whoami"))
{
addExpectedKey("user_id");
setMaxRetries(std::numeric_limits<int>::max());
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 generated code (look in CONTRIBUTING.md and search csapi in it) - this line will be overwritten the next time I run the generator (which can be almost any time).

}