Replaced GAIResolver with twisted.names.client for the default resolver #5053

olivia-fl · 2019-04-12T23:03:31Z

I profiled synapse for a relatively small homeserver that is in several large rooms, and found that a huge chunk of time was being taken up by calls to getaddrinfo:

It turns out that twisted defaults to using twisted.internet._resolver.GAIResolver for resolving hostnames. This is implemented with blocking getaddrinfo calls on a thread pool, but python appears to not allow running concurrent getaddrinfo anyway. This pull request replaces the default relsover with twisted.names.client, which is a user space non-blocking resolver.

Signed-off-by: Benjamin Lee [email protected]

Pull Request Checklist

Pull request is based on the develop branch
Pull request includes a changelog file
Pull request includes a sign off

hawkowl · 2019-04-16T08:42:19Z

@Benjamin-L Hi, thanks for the patch!

I'm going to hold off on merging this for two reasons:

This uncovered a bug in Twisted's DNS implementation on Python 3. (https://twistedmatrix.com/trac/ticket/9627#ticket)
The reason why Twisted (and Synapse) uses GAI as the default is that it makes DNS issues more debuggable for the average user. By using GAI, and therefore the operating system's implementation, cache, and that sort of thing, a user can debug DNS issues with operating system tools. If Synapse (or Twisted) quietly uses its own implementation, they require knowledge of Twisted's DNS system to debug issues.

As such, I would like if this was put behind a configuration option, so people can opt into it instead.

olivia-fl · 2019-04-16T21:02:55Z

That seems pretty reasonable. Amusingly, I ended up discovering that exact issue when trying to debug the sytest failures, but didn't find anything on the issue tracker for it. It's good to know it's already known.

Another thing that I've found out since submitting this is that whether or not getaddrinfo releases the GIL varies depending on OS. On OS's where it doesn't lock, I'm not even sure it's actually a bottleneck, since the profiler samples would be on different threads that wouldn't block the main thread. For that we'd need a good way of measuring how much time is actually spend waiting on DNS queries, and I'm not familiar enough with python to know what a good way of doing that is.

Putting this behind a config option seems like a good choice. I'm going to try to implement that and add a new commit. I'll call the option no_getaddrinfo for now, but there's probably a better name.

codecov · 2019-04-16T21:40:39Z

Codecov Report

Merging #5053 into develop will decrease coverage by <.01%.
The diff coverage is 50%.

@@             Coverage Diff             @@
##           develop    #5053      +/-   ##
===========================================
- Coverage    61.57%   61.56%   -0.01%     
===========================================
  Files          332      332              
  Lines        34265    34267       +2     
  Branches      5648     5648              
===========================================
+ Hits         21097    21098       +1     
- Misses       11654    11655       +1     
  Partials      1514     1514

codecov · 2019-04-16T21:40:39Z

Codecov Report

Merging #5053 into develop will decrease coverage by 0.58%.
The diff coverage is 50%.

@@             Coverage Diff             @@
##           develop    #5053      +/-   ##
===========================================
- Coverage    62.15%   61.56%   -0.59%     
===========================================
  Files          336      332       -4     
  Lines        34659    34269     -390     
  Branches      5694     5649      -45     
===========================================
- Hits         21541    21099     -442     
- Misses       11583    11656      +73     
+ Partials      1535     1514      -21

synapse/config/server.py

richvdh

+1 to what erik said. Please could you rename the option.

Also looks like a conflict has crept in - please could you merge in latest develop?

Otherwise this looks good to me.

…e config

olivia-fl · 2019-05-13T23:40:36Z

I've pushed the renamed option, as well as adding it to the default config. Do you want me to squash the commits?

richvdh

Thanks again for this. No need to squash the commits (we can do that when we merge the PR), but please take a look at the comments below.

I have a concern, however:

I note that the CI is failing (which is partly because the condition at https://github.com/matrix-org/synapse/pull/5053/files#diff-80846f8c47b65e25b3c6716a3eb1bbc8R135 is reversed, so that the option is being used when it shouldn't, but even so: I wouldn't expect the integration tests to fail).

It looks like, when we switch to twisted.names.client, outbound requests to https://localhost:port are only made to the IPv6 version of localhost (::1), and it seems that sytest is only listening on IPv4. My expectation would be that we would attempt to connect to both IPv4 and IPv6. @hawkowl: is this the manifestation of twisted #9627?

richvdh · 2019-05-14T10:34:47Z

docs/sample_config.yaml

@@ -46,6 +46,12 @@ pid_file: DATADIR/homeserver.pid
 #
 #cpu_affinity: 0xFFFFFFFF

+# Whether to use the default system resolver (getaddrinfo) instead of the twisted


note that this is a generated file (the script to update it is scripts-dev/generate-sample-config). Please could you update the source for it (in synapse/config/server.py) and then run the script to update the sample config?

richvdh · 2019-05-14T10:34:57Z

docs/sample_config.yaml

+# asynchronous dns resolver. Using the async resolver can potentially improve
+# performance, but may also behave different from getaddrinfo.
+# Defaults to True. Uncomment to use the twisted resolver instead.
+#use_getaddrinfo_for_dns: False


while you're changing the source, please could you add a line with just # between the text and the option, for consistency with the rest of the file.

richvdh · 2019-05-14T10:37:51Z

changelog.d/5053.misc

@@ -0,0 +1 @@
+Replace blocking getaddrinfo hostname resolver with the non-blocking resolver from twisted.names.client.


Suggested change

Replace blocking getaddrinfo hostname resolver with the non-blocking resolver from twisted.names.client.

Add an option to replace the blocking `getaddrinfo` hostname resolver with the non-blocking resolver from `twisted.names.client`.

(could call this a feature rather than a misc?)

Also, feel free to add yourself as a contributor here, along the lines of the other entries in CHANGES.md.

richvdh · 2019-05-14T11:24:52Z

synapse/app/_base.py

@@ -127,6 +131,10 @@ def run():
            change_resource_limit(soft_file_limit)
            if gc_thresholds:
                gc.set_threshold(*gc_thresholds)
+
+            if use_getaddrinfo_for_dns:


surely this condition is reversed?

richvdh · 2019-06-27T17:08:06Z

@Benjamin-L any chance of taking another look at this?

richvdh · 2019-07-22T21:01:02Z

I'm going to assume this is abandoned. Please reopen it if not!

olivia-fl force-pushed the avoid-getaddrinfo branch 2 times, most recently from bd0ce97 to 834290c Compare April 12, 2019 23:14

erikjohnston reviewed Apr 17, 2019

View reviewed changes

synapse/config/server.py Outdated Show resolved Hide resolved

richvdh suggested changes May 9, 2019

View reviewed changes

olivia-fl added 4 commits May 13, 2019 16:38

Replaced GAIResolver with twisted.names.client for the default resolver

3354a08

Changelog entry

c138c7a

Enable/disable getaddrinfo depending on a config option

f4a6ab2

renamed no_getaddrinfo to use_getaddrinfo_for_dns and added to exampl…

c0aff07

…e config

olivia-fl force-pushed the avoid-getaddrinfo branch from ca00a32 to c0aff07 Compare May 13, 2019 23:39

richvdh self-requested a review May 14, 2019 10:00

richvdh suggested changes May 14, 2019

View reviewed changes

richvdh closed this Jul 22, 2019

richvdh mentioned this pull request Dec 3, 2019

Federation DNS Lookup fails for healthy domain #5831

Closed

richvdh mentioned this pull request Mar 20, 2020

Pusher stops pushing when it falls behind, with lots of DNSLookupFailed errors #7113

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replaced GAIResolver with twisted.names.client for the default resolver #5053

Replaced GAIResolver with twisted.names.client for the default resolver #5053

olivia-fl commented Apr 12, 2019 •

edited

Loading

hawkowl commented Apr 16, 2019

olivia-fl commented Apr 16, 2019

codecov bot commented Apr 16, 2019

codecov bot commented Apr 16, 2019 •

edited

Loading

richvdh left a comment

olivia-fl commented May 13, 2019

richvdh left a comment

richvdh May 14, 2019

richvdh May 14, 2019

richvdh May 14, 2019

richvdh May 14, 2019

richvdh May 14, 2019

richvdh May 14, 2019

richvdh commented Jun 27, 2019

richvdh commented Jul 22, 2019

		@@ -0,0 +1 @@
		Replace blocking getaddrinfo hostname resolver with the non-blocking resolver from twisted.names.client.

	Replace blocking getaddrinfo hostname resolver with the non-blocking resolver from twisted.names.client.
	Add an option to replace the blocking `getaddrinfo` hostname resolver with the non-blocking resolver from `twisted.names.client`.

Replaced GAIResolver with twisted.names.client for the default resolver #5053

Replaced GAIResolver with twisted.names.client for the default resolver #5053

Conversation

olivia-fl commented Apr 12, 2019 • edited Loading

Pull Request Checklist

hawkowl commented Apr 16, 2019

olivia-fl commented Apr 16, 2019

codecov bot commented Apr 16, 2019

Codecov Report

codecov bot commented Apr 16, 2019 • edited Loading

Codecov Report

richvdh left a comment

Choose a reason for hiding this comment

olivia-fl commented May 13, 2019

richvdh left a comment

Choose a reason for hiding this comment

richvdh May 14, 2019

Choose a reason for hiding this comment

richvdh May 14, 2019

Choose a reason for hiding this comment

richvdh May 14, 2019

Choose a reason for hiding this comment

richvdh May 14, 2019

Choose a reason for hiding this comment

richvdh May 14, 2019

Choose a reason for hiding this comment

richvdh May 14, 2019

Choose a reason for hiding this comment

richvdh commented Jun 27, 2019

richvdh commented Jul 22, 2019

olivia-fl commented Apr 12, 2019 •

edited

Loading

codecov bot commented Apr 16, 2019 •

edited

Loading