-
Notifications
You must be signed in to change notification settings - Fork 40
Fix TCP Requester #recv_reply #105
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
base: master
Are you sure you want to change the base?
Conversation
|
@hsbt I would love to get a review on this |
lib/resolv.rb
Outdated
| raise Errno::ECONNRESET if len_data.nil? | ||
| len = len_data.unpack('n')[0] | ||
| reply = @socks[0].read(len) | ||
| raise Errno::ECONNRESET if reply.nil? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we're at it, could we also fix the cases where len_data.bytesize != 2 and reply.bytesize != len?
Raising Errno::ECONNRESET when it doesn't involve the ECONNRESET errno feels wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comment! You are right that ECONNRESET was probably not the right exception. I changed it to EOFError which is probably not perfect, but much closer to reality.
st0012
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nits on newlines. Otherwise looks good to me 👍
@rhenium Would you mind giving it another look? Thanks
5d37435 to
9c640bd
Compare
I've been seeing some intermittent
NoMethodError undefined method 'unpack' for nilexceptions raised from Resolv.This seems to have been caused by misbehaving DNS servers closing connections before answering. The fix is to simply make sure we were able to read data from the server before trying to interpret it.
The raised
Errno::ECONNRESETwill be caught by the calling#requestmethod and re-raised asResolvTimeout.