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.

Sign up for GitHub

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

Jump to bottom

Fixed dns lookup in case of server close connection with econnrefused #1557

Merged
merged 1 commit into from
Sep 19, 2017
Merged

Fixed dns lookup in case of server close connection with econnrefused #1557

merged 1 commit into from
Sep 19, 2017

Conversation

egobrain
Copy link

@egobrain egobrain commented Sep 1, 2017 •
edited
Loading

In case of ns server responded with {error, econnrefused} we should close udp connection and start new one for next ns server.

In my case there is /etc/resolve.conf with folowing structure

nameserver 127.0.0.1
nameserver 8.8.8.8
search ghcg.com

and i tried to resolve an srv record, but local dns server (127.0.0.1) crashed on request.

$ dig srv _tickdb-qida-ws._tcp.load.ghcg.com

; <<>> DiG 9.10.3-P4-Ubuntu <<>> srv _tickdb-qida-ws._tcp.load.ghcg.com
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 55939
;; flags: qr rd ra; QUERY: 1, ANSWER: 1, AUTHORITY: 4, ADDITIONAL: 7

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 1280
;; QUESTION SECTION:
;_tickdb-qida-ws._tcp.load.ghcg.com. IN SRV

;; ANSWER SECTION:
_tickdb-qida-ws._tcp.load.ghcg.com. 300 IN SRV  1 0 8585 tickdb3.load.zorg.sh.

;; AUTHORITY SECTION:
load.ghcg.com.          300     IN      NS      ns-980.awsdns-58.net.
load.ghcg.com.          300     IN      NS      ns-1638.awsdns-12.co.uk.
load.ghcg.com.          300     IN      NS      ns-235.awsdns-29.com.
load.ghcg.com.          300     IN      NS      ns-1395.awsdns-46.org.

;; ADDITIONAL SECTION:
ns-235.awsdns-29.com.   84551   IN      A       205.251.192.235
ns-235.awsdns-29.com.   81987   IN      AAAA    2600:9000:5300:eb00::1
ns-980.awsdns-58.net.   28641   IN      A       205.251.195.212
ns-980.awsdns-58.net.   21916   IN      AAAA    2600:9000:5303:d400::1
ns-1395.awsdns-46.org.  16772   IN      A       205.251.197.115
ns-1395.awsdns-46.org.  90528   IN      AAAA    2600:9000:5305:7300::1

;; Query time: 188 msec
;; SERVER: 127.0.1.1#53(127.0.1.1)
;; WHEN: Fri Sep 01 11:35:46 MSK 2017
;; MSG SIZE  rcvd: 372

1> inet_res:getbyname("_tickdb-qida-ws._tcp.load.ghcg.com", srv).                                                 
{error, timeout}.

In case of first ns server crashes erlang should use next dns server (8.8.8.8), but request crashes on erlang side with {error,einval} badmatch.

Here is simple example what's going on if we dont close connection after first {error, econnrefused}:

1> Test = fun(Urls) ->
    Buffer = <<178,7,1,0,0,1,0,0,0,0,0,0,17,95,116,105,99,107,100,98,45,113,119,101,98,45,
        104,116,116,112,4,95,116,99,112,4,108,111,97,100,4,103,104,99,103,3,99,111,
        109,0,0,33,0,1>>,
    T = 5000,
    {ok, S} = gen_udp:open(0, [{active,false},binary,inet]),
    lists:map(fun({IP, Port}=Url) ->
        ok = gen_udp:connect(S, IP, Port),
        ok = gen_udp:send(S, IP, Port, Buffer),
        {Url, gen_udp:recv(S, 0, T)}
    end, Urls)
end.

2> Test([{{127,0,0,1}, 53}, {{8,8,8,8}, 53}]).
** exception error: no match of right hand side value {error,einval}

3> Test([{{8,8,8,8}, 53}]).                   
[{{{8,8,8,8},53},
  {ok,{{8,8,8,8},
       53,
       <<178,7,129,128,0,1,0,1,0,0,0,0,17,95,116,105,99,107,
         100,98,45,...>>}}}]

4> Test([{{8,8,8,8}, 53}, {{127,0,0,1}, 53}]).
[{{{8,8,8,8},53},
  {ok,{{8,8,8,8},
       53,
       <<178,7,129,128,0,1,0,1,0,0,0,0,17,95,116,105,99,107,
         100,98,45,...>>}}},
 {{{127,0,0,1},53},{error,econnrefused}}]

The solution is to close udp connection on {error, econnrefused} message

@egobrain egobrain force-pushed the 000-fix-dns-lookup branch 2 times, most recently from 8cb694b to f33b8b8 Compare September 1, 2017 12:54
@IngelaAndin IngelaAndin added the team:PS Assigned to OTP team PS label Sep 1, 2017
RaimoNiskanen
RaimoNiskanen reviewed Sep 13, 2017
View reviewed changes
lib/kernel/src/inet_res.erl Outdated
end};
{S, query_tcp(
TcpTimeout, Id, Buffer, IP, Port, Verbose)};
{error, Reason}=Err when Reason =:= econnrefused ->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is neater with {error, econnrefused} = Err ->

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@RaimoNiskanen
Copy link
Contributor

Looks right to me

@RaimoNiskanen RaimoNiskanen added the testing currently being tested, tag is used by OTP internal CI label Sep 13, 2017
@RaimoNiskanen RaimoNiskanen merged commit 6d11362 into erlang:master Sep 19, 2017
@RaimoNiskanen RaimoNiskanen removed the testing currently being tested, tag is used by OTP internal CI label Sep 19, 2017
@RaimoNiskanen
Copy link
Contributor

Merged. Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RaimoNiskanen RaimoNiskanen RaimoNiskanen left review comments

Assignees

@RaimoNiskanen RaimoNiskanen

Labels
team:PS Assigned to OTP team PS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@egobrain @RaimoNiskanen @IngelaAndin