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

commented Oct 19, 2015

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.

AccountState: Avoid ConnectionCheck if ETag job was just done.
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

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

Copy link
Collaborator

commented Oct 19, 2015

Please mention the related issue ID in the commit message

#3677

@guruz

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

Copy link
Contributor Author

commented Oct 19, 2015

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

@ogoffart

This comment has been minimized.

Copy link
Collaborator

commented Oct 20, 2015

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.

This comment has been minimized.

Copy link
@phil-davis

phil-davis Oct 23, 2015

Contributor

"was not too far in the past." ?

@guruz

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

Copy link
Collaborator

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 Apr 21, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.