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.

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

AccountState: Avoid ConnectionCheck if ETag job was just done. #3964

Closed
wants to merge 2 commits into from

Conversation

dragotin
Copy link
Contributor

This patch lets a successful etag job check mark a timestamp.
If next time a connection check is requested, it is checked if
the last ETag happened within the last 30 seconds and if so the
connection check can be checked.

This way we avoid half of the PROPFINDs if all goes well.

@ckamm @ogoffart @guruz please check.

This patch lets a successful etag job check mark a timestamp.
If next time a connection check is requested, it is checked if
the last ETag happened within the last 30 seconds and if so the
connection check can be checked.

This way we avoid half of the PROPFINDs if all goes well.
@guruz
Copy link
Contributor

guruz commented Oct 19, 2015

(haven't tested this theory)

With multiple sync folders/accounts the ETag checks will be done sequentially (at least that's how it was before multi-account)

Having a check with <= 30 sec will therefore mostly be false since those checks were sequential and could be longer than 30 sec ago

@guruz
Copy link
Contributor

guruz commented Oct 19, 2015

Please mention the related issue ID in the commit message

#3677

@guruz
Copy link
Contributor

guruz commented Oct 19, 2015

Ah, and the 30 sec should be instead related to remotePollInterval ? (Plus the comment above I mentioned with regards to multiple accounts/folders)

@dragotin
Copy link
Contributor Author

@guruz yes, exactly my thought, it should be the same as remotePollInterval. Will follow up.

@ogoffart
Copy link
Contributor

I think the right fix is not to do the connection check chen the account is online. The etag check will make the connection drop.

* this account.
* The checkConnectivity() method uses the timestamp to save a call to
* the server to validate the connection if the last successful etag job
* is not so lang away.
Copy link
Contributor

Choose a reason for hiding this comment

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

"was not too far in the past." ?

@guruz guruz added this to the 2.1-next milestone Nov 3, 2015
@guruz guruz added bug Performance p2-high Escalation, on top of current planning, release blocker labels Nov 3, 2015
@guruz
Copy link
Contributor

guruz commented Nov 3, 2015

btw, we also must not break

void AccountState::checkConnectivity(CredentialFetchMode credentialsFetchMode)
...
#ifdef Q_OS_WIN
        // There seems to be a bug in Qt on Windows where QNAM sometimes stops
        // working correctly after the computer woke up from sleep. See #2895 #2899
        // and #2973.
        // As an attempted workaround, reset the QNAM regularly if the account is
        // disconnected.
        account()->resetNetworkAccessManager();

@guruz
Copy link
Contributor

guruz commented Nov 4, 2015

@dragotin You had asked me for an explanation on #3964 (comment)

What I meant is that FolderMan::slotEtagPollTimerTimeout() handles the ETagJob for all accounts and it only schedules one job at the same time. So if you have two accounts then your 30 sec check would not work.

I'll have a look at this and possibly fix this here.

@guruz guruz assigned guruz and unassigned dragotin Nov 18, 2015
guruz added a commit that referenced this pull request Nov 18, 2015
@guruz
Copy link
Contributor

guruz commented Nov 18, 2015

Cherry picked into 2.1

@guruz guruz closed this Nov 18, 2015
guruz added a commit that referenced this pull request Nov 18, 2015
@ogoffart ogoffart deleted the avoid_propfinds branch April 21, 2016 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug p2-high Escalation, on top of current planning, release blocker Performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants