Skip to content

Commit

Permalink
feat: make timeout and lifetime settings of DNS resolver individually…
Browse files Browse the repository at this point in the history
… configurable. Fixes #1102
  • Loading branch information
ecederstrand committed Aug 4, 2022
1 parent cc07d08 commit 5b6656d
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 3 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ Change Log
HEAD
----
- Fixed `Protocol.get_free_busy_info()` when called with +100 accounts.
- Allowed configuring DNS timeout for a single nameserver
(`Autodiscovery.DNS_RESOLVER_ATTRS["timeout""]`) and the total query lifetime
(`Autodiscovery.DNS_RESOLVER_LIFETIME`) separately.


4.7.4
Expand Down
7 changes: 4 additions & 3 deletions exchangelib/autodiscover/discovery.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,9 @@ class Autodiscovery:
MAX_REDIRECTS = 10 # Maximum number of URL redirects before we give up
DNS_RESOLVER_KWARGS = {}
DNS_RESOLVER_ATTRS = {
"timeout": AutodiscoverProtocol.TIMEOUT,
"timeout": AutodiscoverProtocol.TIMEOUT / 2.5, # Timeout for query to a single nameserver
}
DNS_RESOLVER_LIFETIME = AutodiscoverProtocol.TIMEOUT # Total timeout for a query in case of multiple nameservers

def __init__(self, email, credentials=None):
"""
Expand Down Expand Up @@ -364,7 +365,7 @@ def _attempt_response(self, url):
def _is_valid_hostname(self, hostname):
log.debug("Checking if %s can be looked up in DNS", hostname)
try:
self.resolver.resolve(f"{hostname}.", "A", lifetime=self.DNS_RESOLVER_ATTRS.get("timeout"))
self.resolver.resolve(f"{hostname}.", "A", lifetime=self.DNS_RESOLVER_LIFETIME)
except DNS_LOOKUP_ERRORS as e:
log.debug("DNS A lookup failure: %s", e)
return False
Expand All @@ -386,7 +387,7 @@ def _get_srv_records(self, hostname):
log.debug("Attempting to get SRV records for %s", hostname)
records = []
try:
answers = self.resolver.resolve(f"{hostname}.", "SRV", lifetime=self.DNS_RESOLVER_ATTRS.get("timeout"))
answers = self.resolver.resolve(f"{hostname}.", "SRV", lifetime=self.DNS_RESOLVER_LIFETIME)
except DNS_LOOKUP_ERRORS as e:
log.debug("DNS SRV lookup failure: %s", e)
return records
Expand Down

6 comments on commit 5b6656d

@adisidis
Copy link

Choose a reason for hiding this comment

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

It will be nice to make this configurable via the Configure/Account objects for future releases.
Dividing the dns resolver timeout with a 2.5 factor will allow 2~ nameservers to timeout before going to the third.
Anyway, I think it is better to stick with dnspythons default value which is 2.0 seconds. Thus- not setting the AutodiscoverProtocol.TIMEOUT in self.DNS_RESOLVER_ATTRS

@adisidis
Copy link

@adisidis adisidis commented on 5b6656d Aug 4, 2022

Choose a reason for hiding this comment

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

It is also possible not to set any timeout (resolver and lifespan) explicitly, and just use dnspython as is. I didn't read about any specific timeout value stratagies in Autodiscover for Exchange ActiveSync developers.
This way, the python dns module will handle it all.
Thanks for your rapid response.

@ecederstrand
Copy link
Owner Author

@ecederstrand ecederstrand commented on 5b6656d Aug 4, 2022

Choose a reason for hiding this comment

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

Thanks for your comments!

I'd like to not clutter the Configuration or Account objects with these settings. They should not be needed by most users, and it would open up for lots of other config options also going in there.

The factor of 2.5 came from the dnspython defaults of 2.0 / 5.0 seconds for the timeout / lifetime settings. In my experience, a 5 second timeout caused too many DNS timeout errors in practice for end users and scripts running 24/7, so I'd like to keep the timeout higher than the default.

If you want to use the default timeout, then remove the timeout key from DNS_RESOLVER_ATTRS. If you want the default lifetime, then set DNS_RESOLVER_LIFETIME to None.

@adisidis
Copy link

Choose a reason for hiding this comment

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

@ecederstrand Got your point, makes sense. Anyway, this change looks much better than the current situation.
Any ETA for the next release?

@ecederstrand
Copy link
Owner Author

Choose a reason for hiding this comment

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

I'm waiting for feedback on #1090 and will release a new version when that is sorted out.

@ecederstrand
Copy link
Owner Author

Choose a reason for hiding this comment

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

Version 4.7.5 has now been released.

Please sign in to comment.