Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

limit total timeout for get_missing_events to 10s #1744

Merged
merged 2 commits into from Jan 5, 2017

Conversation

Projects
None yet
2 participants
Owner

ara4n commented Dec 31, 2016 edited

timeout calls to get_missing_events from the federation client after 10s rather than the current 5 minutes. should improve the workaround for #1733 and #1729 some more.

Contributor

NegativeMjark commented Jan 3, 2017

It might be worth noting in the comment that this disables HTTP client retries entirely.
https://github.com/matrix-org/synapse/blob/master/synapse/http/matrixfederationclient.py#L189

Also worth noting in the comment that this increases the chances of falling back to fetching state, which curiously isn't guarded by the same lock as the get_missing_events.

Also might be worth mentioning in the comment that we don't want to block for a long time because we are holding a lock on the room that could block other transactions that needed to fetch missing events.

Other than lack of comments LGTM.

Owner

ara4n commented Jan 5, 2017

will fix #1741 when merged

Owner

ara4n commented Jan 5, 2017

@NegativeMjark have updated the comment - PTAL

Contributor

NegativeMjark commented Jan 5, 2017

LGTM

@NegativeMjark NegativeMjark merged commit 5175094 into develop Jan 5, 2017

2 of 6 checks passed

Sytest Dendron (Merged PR) Build finished.
Details
Sytest Postgres (Merged PR) Build started sha1 is merged.
Details
Sytest SQLite (Commit) Build #2188 origin/matthew/timeout_get_missing_events in progress...
Details
Sytest SQLite (Merged PR) Build started sha1 is merged.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

NegativeMjark added a commit that referenced this pull request Jan 5, 2017

Merge pull request #1765 from matrix-org/markjh/timeout_get_missing_e…
…vents

cherrypick #1744: limit total timeout for get_missing_events to 10s
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment