Client's Exchange always creates new socket

[miekg]

Client's Exchange always creates new socket

[miekg]

[ Quoting [email protected] in "[dns] Client's Exchange always crea..." ]

There is issue I'm observing while trying to create fast moving tool with your
library (which is great).

On high levels of concurrency, when I let goroutines run wild I'm getting "dial
udp a.b.c.d:53: too many open files" error.

IMO that's due to client's exchange logic that only has single way of doing
things ritght now, "Exchange() -> dial()/send()->write()/receive()->read()"

That will essentially create socket in OS for every operation and send/receive
on it via "send and recv". Sendto and recvfrom in my situation would be most
efficient but syscall lib is very hard to deal with, I'm trying to find simpler
solutions...

IMO other version of Exchange could have additional net.Conn parameter for
clients who send bunch of requests to same address... As coder I'd expect it
this way:

dnsconn := c.Dial(addr)
for _, msg := range queries {
resp, rtt, err := c.ExchangeWith(dnsconn)
if err {
...
}
...
}

Should not be hard to implement... You might suggest different way of handling
that. I understand that retrying logic might require to keep more than one
net.Conn inside of that "dnsconn" structure.

nice idea. I killed the whole retrying stuff - made the code less robust and
is better handled in the client code itself. So that is not an issue (any more).

is ...With the Go idiom for this stuff? Or ExchangeConn?

grtz Miek

[asergeyev]

For basic mitigation of problem "too many open files" defer w.conn.close() (in reply read() ) works for me. But it's a hack.
Ah, I see that Attempts there does not affect connection at all, thanks for the reply!

naming is fine as you suggest, I'm relatively new to Go ;)

[miekg]

[ Quoting [email protected] in "Re: [dns] Client's Exchange always ..." ]

For basic mitigation of problem "too many open files" defer w.conn.close()
works for me.

I.e. you now have a patch for go dns?

Ah, I see that Attempts there does not affect connection at all, thanks for the
reply!

naming is fine as you suggest, I'm relatively new to Go ;)

Regards,

Miek Gieben                                               http://miek.nl

[asergeyev]

I'm not sure if it's ideal place to stick that:

diff --git a/client.go b/client.go
index 7842cfc..52b0b69 100644
--- a/client.go
+++ b/client.go
@@ -124,6 +124,7 @@ func (w *reply) read(p []byte) (n int, err error) {
    if attempts == 0 {
        attempts = 1
    }
+   defer w.conn.Close()
    switch w.client.Net {
    case "tcp", "tcp4", "tcp6":
        for a := 0; a < attempts; a++ {

I can try on ExchangeConn too, just always concerned about "to code something you have to understand it"...

Patching reminds me adding gas in the car that won't start :)

[miekg]

[ Quoting [email protected] in "Re: [dns] Client's Exchange always ..." ]

I'm not sure if it's ideal place to stick that:

ack. Well I've run into the running out of descriptors too. So I'll add
something like this. Need to be careful with for instance AXFR, which
needs a longlasting open tcp connection.

diff --git a/client.go b/client.go
index 7842cfc..52b0b69 100644
--- a/client.go
+++ b/client.go
@@ -124,6 +124,7 @@ func (w *reply) read(p []byte) (n int, err error) {
if attempts == 0 {
attempts = 1
}

• defer w.conn.Close()
  switch w.client.Net {
  case "tcp", "tcp4", "tcp6":
  for a := 0; a < attempts; a++ {

I can try on ExchangeConn too, just always concerned about "to code something
you have to understand it"...

Patching reminds me adding gas in the car that won't start :)

—
Reply to this email directly or view it on GitHub.

Regards,

Miek Gieben                                               http://miek.nl

[asergeyev]

Cleaner way: https://gist.github.com/4657796

[miekg]

[ Quoting [email protected] in "Re: [dns] Client's Exchange always ..." ]

And looks like connection is there either way... It could be without defer
after w.rreceive in Exchange function, cleaner that way.
https://gist.github.com/4657796

think that should just work.

grtz Miek

[miekg]

[ Quoting [email protected] in "Re: [dns] Client's Exchange always ..." ]

And looks like connection is there either way... It could be without defer
after w.rreceive in Exchange function, cleaner that way.
https://gist.github.com/4657796

Just pushed to master - let me know what happens :)

Regards,

Miek Gieben                                               http://miek.nl

[miekg]

[ Quoting [email protected] in "[dns] Client's Exchange always crea..." ]

IMO other version of Exchange could have additional net.Conn parameter for
clients who send bunch of requests to same address... As coder I'd expect it
this way:

dnsconn := c.Dial(addr)
for _, msg := range queries {
resp, rtt, err := c.ExchangeWith(dnsconn)
if err {
...
}
...
}

I might still add this, but I want to keep the Go DNS api as small as possible,
so if the commited patch also works....

grtz Miek

[asergeyev]

I'm not sure what risk is there if you just make reply struct and methods public

[miekg]

[ Quoting [email protected] in "Re: [dns] Client's Exchange always ..." ]

I'm not sure what risk is there if you just make reply struct and methods
public

There is no inherent risk.

But those methods work on the private reply, so that must be public too. So
my (counter)question becomes: do you *really need them? (Or do you just want
them :) )

Regards,

Miek Gieben                                               http://miek.nl

[asergeyev]

Not really. But it would suffice for ExchangeConn way and also give ability to do send/receive asyncronously and/or in different goroutines, which is interesting feature to have

[miekg]

[ Quoting [email protected] in "Re: [dns] Client's Exchange always ..." ]

Not really. But it would suffice for ExchangeConn way and also give ability to
do send/receive asyncronously and/or in different goroutines, which is
interesting feature to have

Agreed. I've added an ExchangeConn(). Thanks for the suggestion.

grtz Miek

[asergeyev]

Thanks!

[asergeyev]

func (c *Client) ExchangeConn(m *Msg, s net.Conn) (r *Msg, rtt time.Duration, err error) {
    w := &reply{client: c}

btw, it's missing "conn:s" there (sorry just got to try it)

[miekg]

[ Quoting [email protected] in "Re: [dns] Client's Exchange always ..." ]

func (c *Client) ExchangeConn(m *Msg, s net.Conn) (r *Msg, rtt time.Duration, err error) {
w := &reply{client: c}

btw, it's missing "conn:s" there

duh. Thanks.

grtz Miek

[miekg]

[ Quoting [email protected] in "Re: [dns] Client's Exchange always ..." ]

Thanks!

Just thinking about is some more. Would a net/http approach also work (for you)?
With an extra method Hijack() that hijacks the connection and saves it in a
*Client.

Down side is that the addr argument in Exchange would be unused. And that
*Client now knows something about connections...

grtz Miek

[asergeyev]

it will be ok too

[miekg]

Thanks. After some more thought, I think the ExchangeConn is the cleanest solution. A real solution would be to implement a interface which would allow you to plug in your own resolver. But even with that I'm not sure if the added flexibility is worth the trouble.

[asergeyev]

My small worry is that there is no async "send ton of questions" and "receive answers and match ports/IDs to know which are which" solution in Go yet. It's most efficient way to deal with DNS, I cannot be sure but I would've implemented client with DNS-prefetch this way. But it's also not feature that most users of your lib need...

[miekg]

[ Quoting [email protected] in "Re: [dns] Client's Exchange always ..." ]

My small worry is that there is no async "send ton of questions" and "receive
answers and match ports/IDs to know which are which" solution in Go yet. It's
most efficient way to deal with DNS, I cannot be sure but I would've
implemented client with DNS-prefetch this way. But it's also not feature that
most users of your lib need...

Putting async stuff in package is generaly frowned upon in Go - as the
primitives are so easy, it is better to let the user of the package decide
how to use it asynchronously.

A client.Prefetch (or something) might be worth while addition. It should
also track indentical question and stuff like that. Or what kind of function
signature had you in mind?

For my unbound package the same requirements might be in order.

Regards,

Miek Gieben                                               http://miek.nl

[asergeyev]

Small consequence of ExchangeConn - if you receive a response after you already think you got a timeout and send another query in - client will accept it no matter what. Send/Receive can benefit from a check on transaction ID field, will also help with probability of spoofing.

[miekg]

[ Quoting [email protected] in "Re: [dns] Client's Exchange always ..." ]

Small consequence of ExchangeConn - if you receive a response after you already
think you got a timeout and send another query in - client will accept it no
matter what. Send/Receive can benefit from a check on transaction ID field,
will also help with probability of spoofing.

We have DNSSEC to solve this -- although this dns package does not validate,
you'll need to implement it yourself or look at github.com/miekg/unbound.

I was more after if you have thoughts on how to structure "send 1000 questions
and get a 1000 responses" in a neat way. If you say just use ExchangeConn I'm
not sure what to add as a library author?

Regards,

Miek Gieben                                               http://miek.nl

[asergeyev]

I'm not sure. Something should be able to get params for many exchanges and return list of responses, even if caller should be responsible for matching "which response is which"

[miekg]

[ Quoting [email protected] in "Re: [dns] Client's Exchange always ..." ]

I'm not sure. Something should be able to get params for many exchanges and
return list of responses, even if caller should be responsible for matching
"which response is which"

Are you building such a thing yourself atm? Otherwise I'm happy to look at
your finished code and then decide if something like that is a good
addition to go dns?

(If you need help, just ask)

Regards,

Miek Gieben                                               http://miek.nl

[asergeyev]

Sure, will do. So far did not run into something that I cannot live without (ExchangeConn is something I benefitted from though)

[detailyang]

ExchangeConn is not concurrent friendly which can receive other DNS query response