connection: emit networkError in all cases#683
Conversation
KitsuneRal
left a comment
KitsuneRal
left a comment
There was a problem hiding this comment.
I like the direction but the devil's in the details.
| emit networkError(job->errorString(), job->rawDataSample(), | ||
| job->maxRetries(), -1); | ||
| else | ||
| if (job->error() != BaseJob::StatusCode::NetworkError) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
- 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. - Call
job->setMaxRetries()on the job object returned bycallApi(). This is safe to do at any point in the job's lifecycle, no need to delay the job execution. - Delete emission of
networkError()(you already did it here).
4a74de6 to
dcae1dc
Compare
dcae1dc to
01ebbc3
Compare
KitsuneRal
left a comment
KitsuneRal
left a comment
There was a problem hiding this comment.
See below. Also: you accidentally added the temporary .swp file to the PR, please remove it.
| makePath("/_matrix/client/v3", "/account/whoami")) | ||
| { | ||
| addExpectedKey("user_id"); | ||
| setMaxRetries(std::numeric_limits<int>::max()); |
There was a problem hiding this comment.
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).
| emit networkError(job->errorString(), job->rawDataSample(), | ||
| job->maxRetries(), -1); | ||
| else | ||
| if (job->error() != BaseJob::StatusCode::NetworkError) |
There was a problem hiding this comment.
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
- 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. - Call
job->setMaxRetries()on the job object returned bycallApi(). This is safe to do at any point in the job's lifecycle, no need to delay the job execution. - Delete emission of
networkError()(you already did it here).
2 participants
Things brings networkError on parity with requestFailed(), which is emitted for every job that goes through callApi()