From ab1aae2bb91e78acd112d72255a0aac9f6df237d Mon Sep 17 00:00:00 2001 From: Tom Thorogood Date: Tue, 18 Apr 2023 12:40:28 +0930 Subject: [PATCH 1/2] Remove SingleInflight support from Client Callers should instead implement their own in flight query caching. --- client.go | 44 ++++++++++++---------------------- client_test.go | 52 ---------------------------------------- singleinflight.go | 61 ----------------------------------------------- 3 files changed, 15 insertions(+), 142 deletions(-) delete mode 100644 singleinflight.go diff --git a/client.go b/client.go index 9051ae007..25f1fbf5c 100644 --- a/client.go +++ b/client.go @@ -6,7 +6,6 @@ import ( "context" "crypto/tls" "encoding/binary" - "fmt" "io" "net" "strings" @@ -56,14 +55,20 @@ type Client struct { // Timeout is a cumulative timeout for dial, write and read, defaults to 0 (disabled) - overrides DialTimeout, ReadTimeout, // WriteTimeout when non-zero. Can be overridden with net.Dialer.Timeout (see Client.ExchangeWithDialer and // Client.Dialer) or context.Context.Deadline (see ExchangeContext) - Timeout time.Duration - DialTimeout time.Duration // net.DialTimeout, defaults to 2 seconds, or net.Dialer.Timeout if expiring earlier - overridden by Timeout when that value is non-zero - ReadTimeout time.Duration // net.Conn.SetReadTimeout value for connections, defaults to 2 seconds - overridden by Timeout when that value is non-zero - WriteTimeout time.Duration // net.Conn.SetWriteTimeout value for connections, defaults to 2 seconds - overridden by Timeout when that value is non-zero - TsigSecret map[string]string // secret(s) for Tsig map[], zonename must be in canonical form (lowercase, fqdn, see RFC 4034 Section 6.2) - TsigProvider TsigProvider // An implementation of the TsigProvider interface. If defined it replaces TsigSecret and is used for all TSIG operations. - SingleInflight bool // if true suppress multiple outstanding queries for the same Qname, Qtype and Qclass - group singleflight + Timeout time.Duration + DialTimeout time.Duration // net.DialTimeout, defaults to 2 seconds, or net.Dialer.Timeout if expiring earlier - overridden by Timeout when that value is non-zero + ReadTimeout time.Duration // net.Conn.SetReadTimeout value for connections, defaults to 2 seconds - overridden by Timeout when that value is non-zero + WriteTimeout time.Duration // net.Conn.SetWriteTimeout value for connections, defaults to 2 seconds - overridden by Timeout when that value is non-zero + TsigSecret map[string]string // secret(s) for Tsig map[], zonename must be in canonical form (lowercase, fqdn, see RFC 4034 Section 6.2) + TsigProvider TsigProvider // An implementation of the TsigProvider interface. If defined it replaces TsigSecret and is used for all TSIG operations. + + // SingleInflight previously serialised multiple concurrent queries for the + // same Qname, Qtype and Qclass to ensure only one would be in flight at a + // time. + // + // Deprecated: This is a no-op. Callers should implement their own in flight + // query caching. + SingleInflight bool } // Exchange performs a synchronous UDP query. It sends the message m to the address @@ -185,26 +190,7 @@ func (c *Client) ExchangeWithConn(m *Msg, conn *Conn) (r *Msg, rtt time.Duration return c.exchangeWithConnContext(context.Background(), m, conn) } -func (c *Client) exchangeWithConnContext(ctx context.Context, m *Msg, conn *Conn) (r *Msg, rtt time.Duration, err error) { - if !c.SingleInflight { - return c.exchangeContext(ctx, m, conn) - } - - q := m.Question[0] - key := fmt.Sprintf("%s:%d:%d", q.Name, q.Qtype, q.Qclass) - r, rtt, err, shared := c.group.Do(key, func() (*Msg, time.Duration, error) { - // When we're doing singleflight we don't want one context cancellation, cancel _all_ outstanding queries. - // Hence we ignore the context and use Background(). - return c.exchangeContext(context.Background(), m, conn) - }) - if r != nil && shared { - r = r.Copy() - } - - return r, rtt, err -} - -func (c *Client) exchangeContext(ctx context.Context, m *Msg, co *Conn) (r *Msg, rtt time.Duration, err error) { +func (c *Client) exchangeWithConnContext(ctx context.Context, m *Msg, co *Conn) (r *Msg, rtt time.Duration, err error) { opt := m.IsEdns0() // If EDNS0 is used use that for size. if opt != nil && opt.UDPSize() >= MinMsgSize { diff --git a/client_test.go b/client_test.go index ff0c21872..af1bd23ee 100644 --- a/client_test.go +++ b/client_test.go @@ -679,58 +679,6 @@ func TestTimeout(t *testing.T) { }) } -// Check that responses from deduplicated requests aren't shared between callers -func TestConcurrentExchanges(t *testing.T) { - cases := make([]*Msg, 2) - cases[0] = new(Msg) - cases[1] = new(Msg) - cases[1].Truncated = true - - for _, m := range cases { - mm := m // redeclare m so as not to trip the race detector - handler := func(w ResponseWriter, req *Msg) { - r := mm.Copy() - r.SetReply(req) - - w.WriteMsg(r) - } - - HandleFunc("miek.nl.", handler) - defer HandleRemove("miek.nl.") - - s, addrstr, _, err := RunLocalUDPServer(":0") - if err != nil { - t.Fatalf("unable to run test server: %s", err) - } - defer s.Shutdown() - - m := new(Msg) - m.SetQuestion("miek.nl.", TypeSRV) - - c := &Client{ - SingleInflight: true, - } - // Force this client to always return the same request, - // even though we're querying sequentially. Running the - // Exchange calls below concurrently can fail due to - // goroutine scheduling, but this simulates the same - // outcome. - c.group.dontDeleteForTesting = true - - r := make([]*Msg, 2) - for i := range r { - r[i], _, _ = c.Exchange(m.Copy(), addrstr) - if r[i] == nil { - t.Errorf("response %d is nil", i) - } - } - - if r[0] == r[1] { - t.Errorf("got same response, expected non-shared responses") - } - } -} - func TestExchangeWithConn(t *testing.T) { HandleFunc("miek.nl.", HelloServer) defer HandleRemove("miek.nl.") diff --git a/singleinflight.go b/singleinflight.go deleted file mode 100644 index febcc300f..000000000 --- a/singleinflight.go +++ /dev/null @@ -1,61 +0,0 @@ -// Copyright 2013 The Go Authors. All rights reserved. -// Use of this source code is governed by a BSD-style -// license that can be found in the LICENSE file. - -// Adapted for dns package usage by Miek Gieben. - -package dns - -import "sync" -import "time" - -// call is an in-flight or completed singleflight.Do call -type call struct { - wg sync.WaitGroup - val *Msg - rtt time.Duration - err error - dups int -} - -// singleflight represents a class of work and forms a namespace in -// which units of work can be executed with duplicate suppression. -type singleflight struct { - sync.Mutex // protects m - m map[string]*call // lazily initialized - - dontDeleteForTesting bool // this is only to be used by TestConcurrentExchanges -} - -// Do executes and returns the results of the given function, making -// sure that only one execution is in-flight for a given key at a -// time. If a duplicate comes in, the duplicate caller waits for the -// original to complete and receives the same results. -// The return value shared indicates whether v was given to multiple callers. -func (g *singleflight) Do(key string, fn func() (*Msg, time.Duration, error)) (v *Msg, rtt time.Duration, err error, shared bool) { - g.Lock() - if g.m == nil { - g.m = make(map[string]*call) - } - if c, ok := g.m[key]; ok { - c.dups++ - g.Unlock() - c.wg.Wait() - return c.val, c.rtt, c.err, true - } - c := new(call) - c.wg.Add(1) - g.m[key] = c - g.Unlock() - - c.val, c.rtt, c.err = fn() - c.wg.Done() - - if !g.dontDeleteForTesting { - g.Lock() - delete(g.m, key) - g.Unlock() - } - - return c.val, c.rtt, c.err, c.dups > 0 -} From 5042a659a8aa1760c8ac6cc5d96a7a693f5fb865 Mon Sep 17 00:00:00 2001 From: Tom Thorogood Date: Fri, 21 Apr 2023 11:55:42 +0930 Subject: [PATCH 2/2] Add doc link to Github issue --- client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client.go b/client.go index 25f1fbf5c..2cdd49af1 100644 --- a/client.go +++ b/client.go @@ -67,7 +67,7 @@ type Client struct { // time. // // Deprecated: This is a no-op. Callers should implement their own in flight - // query caching. + // query caching if needed. See github.com/miekg/dns/issues/1449. SingleInflight bool }