Limit number of concurrent DNS queries#1062
Conversation
Signed-off-by: Santhosh Manohar <santhosh@docker.com>
resolver.go
Outdated
| } | ||
|
|
||
| func (r *resolver) concurrentQueryInc() bool { | ||
| for { |
There was a problem hiding this comment.
Why don't we just use a mutex? This is more overhead than just using a mutex
|
LGTM pending CI green |
| }() | ||
| err = co.WriteMsg(query) | ||
| if err != nil { | ||
| r.concurrentQueryDec() |
There was a problem hiding this comment.
r.concurrentQueryDec() is executed either if err == nil (line 400) or if err != nil (here).
Therefore, shouldn't we move it before the call err = co.WriteMsg(query) ?
There was a problem hiding this comment.
@aboch In line 400, concurrentQueryDec() will be called for both err = nil and err != nil because once we are out of the read() the current query handling is done, either with a success or failure. But here if the write fails we continue to the next element in the for loop. So the concurrentQueryDec() has to be done only in case of failure.
There was a problem hiding this comment.
What I am trying to say is that once the code reaches line 386, it will decrement the counter anyway.
It does not matter that it does it after an unsuccessful co.WriteMsg(query) or after the call to co.ReadMsg().
The logic seems to be that once r.concurrentQueryInc() == true, r.concurrentQueryDec() will be executed no matter what.
So I was suggesting to remove the two calls to r.concurrentQueryDec() at line 394 and 400 and add one at line 386.
There was a problem hiding this comment.
But it looks like the decrement must be done only after co.WriteMsg() or co.ReadMsg() has been executed ? Then my comment does not apply.
There was a problem hiding this comment.
But it looks like the decrement must be done only after co.WriteMsg() or co.ReadMsg() has been executed ?
Yes. Between co.WriteMsg() and co.ReadMsg() is where the query is forwarded to external DNS server and the reply is accepted. In the case of concurrent calls to ServeDNS this is the block that we want to restrict to maxConcurrent instances.
|
LGTM |
Signed-off-by: Santhosh Manohar santhosh@docker.com