Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

Conversation

@ShadowJonathan
Copy link
Contributor

@ShadowJonathan ShadowJonathan commented Apr 9, 2021

Fixes #9774

Adds a stricter timeout to resolve_service that returns in 50s (smaller than the previous 60s, and the overarching 60s of timeout a federation transaction has)

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
  • Pull request includes a sign off
  • Code style is correct (run the linters)

Signed-off-by: Jonathan de Jong <jonathan@automatia.nl>

try:
answers, _, _ = await make_deferred_yieldable(
self._dns_client.lookupService(service_name)
self._dns_client.lookupService(service_name, timeout=(1, 3, 11, 35))
Copy link
Contributor Author

@ShadowJonathan ShadowJonathan Apr 9, 2021

Choose a reason for hiding this comment

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

1 + 3 + 11 + 35 = 50


By the way, i saw somewhere another federation-esc timeout of 30 seconds, shouldn't this timeout sum become 25 seconds? Or is 50 seconds acceptable?

Copy link
Member

Choose a reason for hiding this comment

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

so what's going on here? we're trying to change the timeout so that it's less than the total federation request timeout, so that we get a useful error message?

if so, it would be useful to say that in a comment.

(looks like the defaults are (1, 3, 11, 45). could you include that in the comment too)

else:
raise e
except defer.TimeoutError as e:
raise defer.TimeoutError(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if this exception should raise "all the way" here via this, but at least it has different wording.

@anoadragon453 anoadragon453 requested a review from a team April 9, 2021 10:49
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

^

@richvdh
Copy link
Member

richvdh commented Apr 9, 2021

also: CI is failing.

@erikjohnston erikjohnston added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Jun 16, 2021
@richvdh
Copy link
Member

richvdh commented Aug 5, 2021

closing this as it seems to be on hold for now. We can reopen if you want to pick it up again.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

synapse.http.federation.srv_resolver.SrvResolver.resolve_service isn't able to "timeout" properly, and thus stalls federation

3 participants