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

Improve handling of DNS changes by revalidating pooled SocketsHttpHandler connections periodically #60390

Open
antonfirsov opened this issue Oct 14, 2021 · 5 comments
Labels
area-System.Net.Http enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@antonfirsov
Copy link
Member

antonfirsov commented Oct 14, 2021

Copying @geoffkizer 's idea from dotnet/dotnet-api-docs#7285 (comment) into a new issue so we can discuss and track it:

The problem occurs when you switch your DNS mappings and remove an existing IP entry (or entries), but the server at that IP address continues to handle requests on the previously-established connections. In that case, SocketsHttpHandler will just keep using those previously-established connections, unaware that they are no longer valid according to the latest DNS values.

PooledConnectionLifetime was introduced to solve this problem. It does so by simply discarding connections once they hit a certain age. Eventually the old connections will be discarded and new ones will be created, and the new connections will respect the new DNS values when they are created.

This isn't ideal for a couple reasons:
(1) Connections get discarded regardless of whether they are still valid per current DNS or not.
(2) This introduces a trade-off between performance and correctness. If you want SocketsHttpHandler to respond quickly to DNS changes, then you need to set this to a relatively small value. But the smaller you set it, the more often you'll need to create new connections, which will impact performance.

On top of that, we clearly don't do a good job of explaining this.

I wonder if we could handle this better in SocketsHttpHandler. One idea is to add a new setting like PooledConnectionDNSRevalidationPeriod. This would be similar to PooledConnectionLifetime, except that when the specified time period expires, we would "revalidate" the connection by checking whether the IP address used to establish it is still valid in DNS or not. If it is, we can keep using it. If not, just discard it, like we do with PooledConnectionLifetime today.

The nice thing about this approach is that, assuming the revalidation logic is relatively cheap, we could enable this by default with a relatively low time period, such that it would hopefully just work for most users. If you don't care about DNS changes, then there's a small added cost to the revalidation, but hopefully it's so small you wouldn't notice (and you can always disable it completely if you want). If you do care about DNS changes, then the default behavior hopefully works reasonably well for you, and you can tune it further if necessary.

@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Net.Http untriaged New issue has not been triaged by the area owner labels Oct 14, 2021
@ghost
Copy link

ghost commented Oct 14, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Putting @geoffkizer 's idea from dotnet/dotnet-api-docs#7285 (comment) into a new issue so we can discuss and track it:

The problem occurs when you switch your DNS mappings and remove an existing IP entry (or entries), but the server at that IP address continues to handle requests on the previously-established connections. In that case, SocketsHttpHandler will just keep using those previously-established connections, unaware that they are no longer valid according to the latest DNS values.

PooledConnectionLifetime was introduced to solve this problem. It does so by simply discarding connections once they hit a certain age. Eventually the old connections will be discarded and new ones will be created, and the new connections will respect the new DNS values when they are created.

This isn't ideal for a couple reasons:
(1) Connections get discarded regardless of whether they are still valid per current DNS or not.
(2) This introduces a trade-off between performance and correctness. If you want SocketsHttpHandler to respond quickly to DNS changes, then you need to set this to a relatively small value. But the smaller you set it, the more often you'll need to create new connections, which will impact performance.

On top of that, we clearly don't do a good job of explaining this.

I wonder if we could handle this better in SocketsHttpHandler. One idea is to add a new setting like PooledConnectionDNSRevalidationPeriod. This would be similar to PooledConnectionLifetime, except that when the specified time period expires, we would "revalidate" the connection by checking whether the IP address used to establish it is still valid in DNS or not. If it is, we can keep using it. If not, just discard it, like we do with PooledConnectionLifetime today.

The nice thing about this approach is that, assuming the revalidation logic is relatively cheap, we could enable this by default with a relatively low time period, such that it would hopefully just work for most users. If you don't care about DNS changes, then there's a small added cost to the revalidation, but hopefully it's so small you wouldn't notice (and you can always disable it completely if you want). If you do care about DNS changes, then the default behavior hopefully works reasonably well for you, and you can tune it further if necessary.

Author: antonfirsov
Assignees: -
Labels:

area-System.Net.Http, untriaged

Milestone: -

@geoffkizer geoffkizer changed the title Revalidate pooled SocketsHttpHandler connections periodically Improve handling of DNS changes by revalidating pooled SocketsHttpHandler connections periodically Oct 16, 2021
@karelz karelz added enhancement Product code improvement that does NOT require public API changes/additions and removed untriaged New issue has not been triaged by the area owner labels Oct 26, 2021
@karelz karelz added this to the Future milestone Oct 26, 2021
@karelz
Copy link
Member

karelz commented Oct 26, 2021

Triage: If we have DNS client (with TTL) - #19443, we won't need this.

edit by Cory: the full strategy we agreed to is in #60390 (comment).

@geoffkizer
Copy link
Contributor

Even if we could get TTL info from DNS (and I'm skeptical we will be able to do this any time soon), the ability to "revalidate" a connection is still useful.

With TTL info, we would know when a connection may have become invalid because the DNS info has changed. But just because the TTL has expired doesn't necessarily mean the DNS info has actually changed. Most of the time, it's the same, with an extended TTL.

If we always just drop the connection when the DNS TTL expires, then we still suffer from this problem above:

Connections get discarded regardless of whether they are still valid per current DNS or not.

So I think the ideal solution would be having TTL info + revalidation -- that is, when the TTL expires, we revalidate the DNS info exactly as described above.

In the absence of TTL info, it seems like having the user provide something like PooledConnectionDNSRevalidationPeriod is a pretty good fallback. Roughly speaking, the user is manually giving us the TTL info instead of automatically retrieving the TTL from DNS.

@scalablecory
Copy link
Contributor

So I think the ideal solution would be having TTL info + revalidation -- that is, when the TTL expires, we revalidate the DNS info exactly as described above.

In the absence of TTL info, it seems like having the user provide something like PooledConnectionDNSRevalidationPeriod is a pretty good fallback. Roughly speaking, the user is manually giving us the TTL info instead of automatically retrieving the TTL from DNS.

The triage comment does not reflect this, but this is the agreement we came to during triage.

@Gladskih
Copy link

Gladskih commented Jul 9, 2024

DNS is not TCP. DNS TTL is not a reason to close a connection. If a server is not able to handle connection anymore it should close it. If it does not - it a bug of the server, not a client. "PooledConnectionLifetime workaround" is just a step in a wrong direction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Net.Http enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

No branches or pull requests

5 participants