search

alexdalitz / dnsruby

Dnsruby is a feature-complete DNS(SEC) client for Ruby, as used by many of the world's largest DNS registries and the OpenDNSSEC project
Other
194 stars 77 forks source link

PacketSender - set tcp socket connect_timeout to match packet_timeout #151

Closed seandilda closed 4 years ago

seandilda commented 5 years ago

Without this, a Resolver pointing to an off-line DNS server will wait over 2 minutes before timing out.

coveralls commented 5 years ago

Coverage Status

Coverage increased (+0.1%) to 78.085% when pulling 71d82bfe50a29075a193b5c6b8991f432522ae44 on seandilda:packet_timeout into 06303437027149fafe78fd09249677ac55d4db34 on alexdalitz:master.

seandilda commented 5 years ago

All tests pass when I run them locally. Alex, are you able to reproduce the test failures?

alexdalitz commented 5 years ago

Don't worry about Travis - it looks like it's complaining about an old version of Bundler - I'll get that fixed up. Your patch looks good - thanks for that! I'm a little snowed under just now, but I'll try to get the tests fixed up and your patch tested and merged some time today, possible tomorrow. Thanks!

keithrbennett commented 5 years ago

@alexdalitz Only the first Ruby, Ruby 2.0.0 had the error relating to the Bundler versions (maybe we should remove that version from the tests anyway?). The other Ruby version tests had a different error, which on very brief review looked to me that they might be related to the change:

  1) Failure:
TestResolver#test_send_plain_message [/home/travis/build/alexdalitz/dnsruby/test/tc_resolver.rb:78]:
Expected no error but got a Dnsruby::ServFail:
Dnsruby::ServFail
  2) Error:
TestRrOpt#test_plain_respects_bufsize:
NoMethodError: undefined method `header' for nil:NilClass
    /home/travis/build/alexdalitz/dnsruby/test/tc_rr-opt.rb:44:in `block in test_plain_respects_bufsize'
    /home/travis/build/alexdalitz/dnsruby/test/tc_rr-opt.rb:50:in `test_plain_respects_bufsize'
alexdalitz commented 5 years ago

It’s very strange - running the patch locally, with ruby-2.5.1, I get :

1) Failure: TestTCPPipelining#test_TCP_pipelining_socket_eof [/Users/alex/code/dnsruby/test/tc_tcp_pipelining.rb:240]: Exception not nil for msg 0 < 4 requests. Expected # to be nil.

2) Failure: TestTCPPipelining#test_TCP_pipelining_timeout [/Users/alex/code/dnsruby/test/tc_tcp_pipelining.rb:116]: Expected # to be nil.

3) Failure: TestTCPPipelining#test_TCP_pipelining_timeout_in_send [/Users/alex/code/dnsruby/test/tc_tcp_pipelining.rb:116]: Expected # to be nil.

198 runs, 1675 assertions, 3 failures, 0 errors, 0 skips

Indeed, I get these pipelining errors in most Ruby’s, with or without the patch. I’m not quite sure what’s going on there (yet).

I don’t understand why the errors in the Travis ruby-2.5.1 build should be so different to my local ones!

On 15 Jan 2019, at 08:21, Keith Bennett notifications@github.com wrote:

@alexdalitz https://github.com/alexdalitz Only the first Ruby, Ruby 2.0.0 had the error relating to the Bundler versions (maybe we should remove that version from the tests anyway?). The other Ruby version tests had a different error, which on very brief review looked to me that they might be related to the change:

1) Failure: TestResolver#test_send_plain_message [/home/travis/build/alexdalitz/dnsruby/test/tc_resolver.rb:78]: Expected no error but got a Dnsruby::ServFail: Dnsruby::ServFail 2) Error: TestRrOpt#test_plain_respects_bufsize: NoMethodError: undefined method header' for nil:NilClass /home/travis/build/alexdalitz/dnsruby/test/tc_rr-opt.rb:44:inblock in test_plain_respects_bufsize' /home/travis/build/alexdalitz/dnsruby/test/tc_rr-opt.rb:50:in `test_plain_respects_bufsize'

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/alexdalitz/dnsruby/pull/151#issuecomment-454304631, or mute the thread https://github.com/notifications/unsubscribe-auth/AHVi2pKbxUgOclCrnJB9pMgwpuP137d4ks5vDY-ZgaJpZM4Z_7Ix.

alexdalitz commented 4 years ago

I'm really sorry this has taken so long! I'm just about to push this change, and hope to release a new dnsruby soon. Thanks!