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

Remove default timeout on Sync #327

Closed
wants to merge 1 commit into from

Conversation

pik
Copy link
Contributor

@pik pik commented Jan 14, 2017

There is currently no backup strategy for low-bandwidth connections which cannot complete a /sync within localTimeoutMs -- this means that slow connections will never load riot-web as the set of state events even for several popular rooms is very large - this removes the localTimeoutMs entirely from the default /sync calls (it can still be set via client.opts) as per element-hq/element-web#2737 (comment).

@pik pik changed the base branch from master to develop January 14, 2017 20:47
@ara4n
Copy link
Member

ara4n commented Jan 14, 2017

I wonder whether removing the client timeout means that if you lose your connectivity during a sync (e.g. by sleeping your laptop, or switching network, etc), the sync will never timeout and the client will never recover from the outage, wedging solid.

Unless the keepAlives logic is adequate to recover from that scenario...

@kegsay, wdyt?

@pik
Copy link
Contributor Author

pik commented Jan 15, 2017

The browser should fail with either, net::ERR_INCOMPLETE_CHUNKED_ENCODING net::ERR_NETWORK_CHANGED etc. I think? Is there a case where the browser might not set request status to failed ? The errors should be handled here: https://github.com/matrix-org/matrix-js-sdk/blob/develop/src/sync.js#L554 and a sync retried after keepalive?

@kegsay
Copy link
Member

kegsay commented Jan 16, 2017

In practice, browsers do not fail and just wedge entirely. Notably, Safari does this. This is why localTimeoutMs is A Thing. I can't accept this PR because it will break other users.

@pik
Copy link
Contributor Author

pik commented Jan 16, 2017

In practice, browsers do not fail and just wedge entirely. Notably, Safari does this

Really? In my testing both Firefox and Chromium fail correctly, is there an open bug for this in Safari?

At any-rate if this is a possibility we have to contend with - what about hooking into onprogress to debounce the timeout event?

@kegsay
Copy link
Member

kegsay commented Jan 16, 2017

what about hooking into onprogress to debounce the timeout event?

That's what I would prefer.

@richvdh
Copy link
Member

richvdh commented Mar 22, 2017

I think this got superceded by #330, and thence #392.

@richvdh richvdh closed this Mar 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants