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

Alternative TCP DNS server to allow pipelined query handling #1592

Open
bimmlerd opened this issue Aug 8, 2024 · 7 comments
Open

Alternative TCP DNS server to allow pipelined query handling #1592

bimmlerd opened this issue Aug 8, 2024 · 7 comments

Comments

@bimmlerd
Copy link

bimmlerd commented Aug 8, 2024

As previous issues have noted (#646, #1314), for any single TCP connection, DNS queries are handled serially. This is generally not a big problem, as

  1. DNS over TCP is not all that common, and
  2. typically the overhead of establishing the TCP connection after having received a truncated UDP packet means the latency is already high, the serial processing of queries doesn't significantly worsen it.

However, in the specific case of a DNS proxy, the serial processing becomes a significant contributing factor: a proxy has to forward the queries as part of the handler, synchronously. (The current handler interface doesn't seem to allow doing so asynchronously - the ResponseWriter is alive while the handler function is running, but not longer.) Since clients often send two (or more) queries approximately simultaneously, the second query incurs a delay of a full RTT to the upstream server, just waiting for the first query to be handled. Similarly so, any further simultaneous queries incur more RTTs waiting for exchanges with the upstream, which isn't great.

Now, as previous issues have established, changing the existing TCP server to be pipelined is not an option (and probably a breaking change), since the handler would have to deal with the fact that it is being called concurrently and the response writer would to be made thread-safe without losing performance.

Long story short, I'm opening this issue to inquire whether the library would accept a contribution of an alternative TCP server which does do pipelining or whether that would be considered out of scope.

@miekg
Copy link
Owner

miekg commented Aug 13, 2024

It would be nice if this could be done in folded into the current tcp server? Is that doable?

@bimmlerd
Copy link
Author

It would be nice if this could be done in folded into the current tcp server? Is that doable?

@miekg I do not think it's doable, to expand on what @tmthrgd said previously:

  1. The current handler interface is ServeDNS(w ResponseWriter, r *Msg). We would have to ensure that concurrent writes to the ResponseWriter are safe (could compromise performance if we need to lock). Also, ResponseWriter.Write([]byte) becomes an unsafe method, since it's possible to accidentially interleave bytes of multiple messages if the writer doesn't write full messages. Finally, ResponseWriter.Hijack() becomes inherently racy.
  2. It would be a breaking change: There currently was no way the handler function would be running concurrently for the same connection, but if we implement pipelinining this can occur. That seems like a breaking change of API semantics, and hence would have to be opt-in or in a v2 of the module.

If we end up making it a configurable thing on the existing TCP server, I think it will make the implementation more complex than having two separate implementations, but that's mostly a matter of taste.

@miekg
Copy link
Owner

miekg commented Aug 14, 2024

thanks for digging that up.

Does the Golang http impl do pipelining btw? As I've followed that model here a bit?

@bimmlerd
Copy link
Author

bimmlerd commented Aug 14, 2024

thanks for digging that up.

Sure thing.

Does the Golang http impl do pipelining btw? As I've followed that model here a bit?

The short answer is no.

I admit that it's not entirely clear what the correct handler interface would look like. I've thought about it briefly, and I think we can make the handler

type handler interface {
    ServeDNS(*dns.Msg) *dns.Msg
}

The ReponseWriter in http is, afaik, primarily necessary since HTTP is a text streaming protocol. DNS doesn't have this requirement, hence we can "just" return a complete message - side-stepping the problem of writing interleaved message parts.

@miekg
Copy link
Owner

miekg commented Aug 14, 2024

ok, frankly I'm worried about a sprawling API that needs to exist just for tcp pipelinging, assuming you get this for free with QUIC. Also I have a server now (say coredns) and I want to do pipelining : what needs to be changed?

OTOH: pipelning is a nice enhancement, but I'm not sure how much of a perf it will get you (assuming that's what is why you want this?)

@bimmlerd
Copy link
Author

ok, frankly I'm worried about a sprawling API that needs to exist just for tcp pipelinging, assuming you get this for free with QUIC.

Yeah, that's fair and definitely an understandable maintainer's perspective. It may be possible to instead of adding a second TCP server to add another field to the Server struct, say Pipelined bool which enables the pipelining at the cost of complicating the serve loop.

Also I have a server now (say coredns) and I want to do pipelining : what needs to be changed?

If we do the above, Pipelined: true to enable the pipelining and then a lot of difficult thinking about whether your handler is safe to be called in parallel.

OTOH: pipelning is a nice enhancement, but I'm not sure how much of a perf it will get you (assuming that's what is why you want this?)

I work on cilium which uses the Server to implement a DNS proxy. For proxying, pipelining is a big win. Imagine two queries (A/AAAA) arriving over a TCP conn. Without pipelining, the second query waits until the first query is proxied to an upstream, which increases the latency. I think recursive resolvers have the same use case.

@miekg
Copy link
Owner

miekg commented Sep 9, 2024

yeah, pipelining is a win there, is there a way where you can do a small concept PR to see how this looks?

Going further, DoQ is the correct solution here, although there is no std Go support for quic yet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants