Make MaxTCPQueries configurable #673
Conversation
Codecov Report
@@ Coverage Diff @@
## master #673 +/- ##
==========================================
+ Coverage 58.14% 58.15% +0.01%
==========================================
Files 37 37
Lines 10017 10020 +3
==========================================
+ Hits 5824 5827 +3
Misses 3145 3145
Partials 1048 1048
Continue to review full report at Codecov.
|
server.go
Outdated
| @@ -12,9 +12,6 @@ import ( | |||
| "time" | |||
| ) | |||
|
|
|||
| // Maximum number of TCP queries before we close the socket. | |||
| const maxTCPQueries = 128 | |||
There was a problem hiding this comment.
We should expose this in the server, and make 0 mean; disable this.
By removing this the server has no way to kill a client after it things it has enough; Google Public DNS does the same thing btw, so it's not uncommon to have this.
b165d1b to
8a4845c
Compare
server.go
Outdated
| // TODO(miek): make maxTCPQueries configurable? | ||
| for q := 0; q < maxTCPQueries; q++ { | ||
| q := 0 | ||
| for { |
There was a problem hiding this comment.
I think this would come out cleaner written as:
for q := 0; q < srv.MaxTCPQueries || srv.MaxTCPQueries == 0; q++ {
server.go
Outdated
| @@ -303,6 +300,8 @@ type Server struct { | |||
| DecorateReader DecorateReader | |||
| // DecorateWriter is optional, allows customization of the process that writes raw DNS messages. | |||
| DecorateWriter DecorateWriter | |||
| // Maximum number of TCP queries before we close the socket (unlimited if 0). | |||
| MaxTCPQueries int | |||
There was a problem hiding this comment.
I question whether having the default value mean unlimited is best, because it amounts to a behaviour change that could be unexpected. Perhaps it would be best to have 0 mean the current max of 128 and -1 explicitly mean unlimited (maybe even with a const UnlimitedMaxQueries = -1 for clarity). @miekg thougts?
|
Re: 128, ugly but I agree
…On Thu, 10 May 2018, 14:52 Tom Thorogood, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In server.go <#673 (comment)>
:
> @@ -593,8 +592,13 @@ func (srv *Server) serve(w *response) {
timeout := srv.getReadTimeout()
- // TODO(miek): make maxTCPQueries configurable?
- for q := 0; q < maxTCPQueries; q++ {
+ q := 0
+ for {
I think this would come out cleaner written as:
for q := 0; q < srv.MaxTCPQueries || srv.MaxTCPQueries == 0; q++ {
------------------------------
In server.go <#673 (comment)>
:
> @@ -303,6 +300,8 @@ type Server struct {
DecorateReader DecorateReader
// DecorateWriter is optional, allows customization of the process that writes raw DNS messages.
DecorateWriter DecorateWriter
+ // Maximum number of TCP queries before we close the socket (unlimited if 0).
+ MaxTCPQueries int
I question whether having the default value mean unlimited is best,
because it amounts to a behaviour change that could be unexpected. Perhaps
it would be best to have 0 mean the current max of 128 and -1 explicitly
mean unlimited (maybe even with a const UnlimitedMaxQueries = -1 for
clarity). @miekg <https://github.com/miekg> thougts?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#673 (review)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAVkW3NNnwCVzMJz5_mL3WAv0L8tkCjDks5txEY0gaJpZM4T0ocN>
.
|
9b7ec7b to
77075aa
Compare
| @@ -13,7 +13,7 @@ import ( | |||
| "time" | |||
| ) | |||
|
|
|||
| // Maximum number of TCP queries before we close the socket. | |||
| // Default maximum number of TCP queries before we close the socket. | |||
| const maxTCPQueries = 128 | |||
There was a problem hiding this comment.
This should probably be renamed defaultMaxTCPQueries, but I can't say I love the name. Maybe it's fine as it is 🤷♂️.
| @@ -303,6 +303,8 @@ type Server struct { | |||
| DecorateReader DecorateReader | |||
| // DecorateWriter is optional, allows customization of the process that writes raw DNS messages. | |||
| DecorateWriter DecorateWriter | |||
| // Maximum number of TCP queries before we close the socket. Default is maxTCPQueries (unlimited if -1). | |||
| MaxTCPQueries int | |||
There was a problem hiding this comment.
I'm a little hesitant about using a magic number here, but it's not blocking to me. (I still like something like const UnlimitedMaxQueries = -1 simply for the clarity it gives).
No description provided.