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.

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

Also wrap Errno::ETIMEDOUT. #212

Merged
merged 4 commits into from
Mar 29, 2013
Merged

Also wrap Errno::ETIMEDOUT. #212

merged 4 commits into from
Mar 29, 2013

Conversation

mat813
Copy link
Contributor

@mat813 mat813 commented Mar 12, 2013

Got it today in :
Operation timed out
whois (2.7.0) lib/whois/server/adapters/base.rb:195:in `read'

Got it today in :
  Operation timed out
  whois (2.7.0) lib/whois/server/adapters/base.rb:195:in `read'
@rb2k
Copy link

rb2k commented Mar 24, 2013

And while we're add it, you could add Errno::EPIPE too :)

ruby-2.0.0-p0/gems/whois-2.7.0/lib/whois/server/adapters/base.rb:194:in `write': Broken pipe (Errno::EPIPE)
        from /usr/local/rvm/gems/ruby-2.0.0-p0/gems/whois-2.7.0/lib/whois/server/adapters/base.rb:194:in `ask_the_socket'
        from /usr/local/rvm/gems/ruby-2.0.0-p0/gems/whois-2.7.0/lib/whois/server/adapters/base.rb:180:in `query_the_socket'
        from /usr/local/rvm/gems/ruby-2.0.0-p0/gems/whois-2.7.0/lib/whois/server/adapters/verisign.rb:37:in `request'
        from /usr/local/rvm/gems/ruby-2.0.0-p0/gems/whois-2.7.0/lib/whois/server/adapters/base.rb:115:in `block in query'
        from /usr/local/rvm/gems/ruby-2.0.0-p0/gems/whois-2.7.0/lib/whois/server/adapters/base.rb:154:in `buffer_start'
        from /usr/local/rvm/gems/ruby-2.0.0-p0/gems/whois-2.7.0/lib/whois/server/adapters/base.rb:114:in `query'
        from /usr/local/rvm/gems/ruby-2.0.0-p0/gems/whois-2.7.0/lib/whois/client.rb:90:in `block in query'
        from /usr/local/rvm/rubies/ruby-2.0.0-p0/lib/ruby/2.0.0/timeout.rb:65:in `timeout'
        from /usr/local/rvm/gems/ruby-2.0.0-p0/gems/whois-2.7.0/lib/whois/client.rb:87:in `query'

@rb2k
Copy link

rb2k commented Mar 24, 2013

And I think I'd suggest just wrapping "SystemCallError" (http://www.ruby-doc.org/core-2.0/Errno.html).
Most of them are somehow Socket related

@weppos
Copy link
Owner

weppos commented Mar 29, 2013

@rb2k SystemCallError can be a very interesting replacement to all the Errno::* classes, thanks!

@mat813 would you mind to apply the changes to your PR and update the tests accordingly, so that I can merge the PR?

@mat813
Copy link
Contributor Author

mat813 commented Mar 29, 2013

You mean replacing all the Errno::* with SystemCallError ?
Will do.

@weppos
Copy link
Owner

weppos commented Mar 29, 2013

Correct. Do you have any objection on this proposal?

@@ -22,9 +22,7 @@ class SocketHandler
# Array of connection errors to rescue
# and wrap into a {Whois::ConnectionError}
RESCUABLE_CONNECTION_ERRORS = [
Errno::ECONNRESET,
Errno::EHOSTUNREACH,
Errno::ECONNREFUSED,
Copy link
Owner

Choose a reason for hiding this comment

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

I would leave these classes in the spec, to make sure we're not breaking BC with previous versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I was wondering about that.

@mat813
Copy link
Contributor Author

mat813 commented Mar 29, 2013

Nope, Just did it :-)

@mat813
Copy link
Contributor Author

mat813 commented Mar 29, 2013

Hum, travis says I did something wrong, letme check…

@weppos
Copy link
Owner

weppos commented Mar 29, 2013

It looks like there is an issue with the spec for SystemCallError. I have a feeling that SystemCallError wants some param at initialization (but I could be wrong).

@ghost ghost assigned weppos Mar 29, 2013
SystemCallError is a special kind of exception, you should not call it
by yourself :

    1.9.3 (main):0 > SystemCallError.new(1)
    => #<Errno::EPERM: Operation not permitted>
    1.9.3 (main):0 > SystemCallError.new("foo", 1)
    => #<Errno::EPERM: Operation not permitted - foo>
    1.9.3 (main):0 > SystemCallError.new("foo")
    => #<SystemCallError: unknown error - foo>
@mat813
Copy link
Contributor Author

mat813 commented Mar 29, 2013

As a side note, there are some missing requires somewhere in the specs, I couldn't run:

$ rspec spec/whois/server/socket_handler_spec.rb

had to do:

$ ruby -Ilib -rwhois/server/socket_handler -S rspec spec/whois/server/socket_handler_spec.rb

@weppos
Copy link
Owner

weppos commented Mar 29, 2013

I noticed it a couple of times in the specs. This is because of autoload.
Feel free to add

require 'whois/server/socket_handler'

at the top of the spec file.

weppos added a commit that referenced this pull request Mar 29, 2013
Also wrap Errno::ETIMEDOUT.
@weppos weppos merged commit 1ea4035 into weppos:master Mar 29, 2013
weppos added a commit that referenced this pull request Mar 29, 2013
@mat813 mat813 deleted the ETIMEDOUT branch March 29, 2013 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants