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

Fix peeraddr raises Errno::EINVAL inside reactor #2014

Closed
wants to merge 1 commit into from

Conversation

hahmed
Copy link
Contributor

@hahmed hahmed commented Oct 6, 2019

Hey @nateberkopec

I had a look into the issue #1564 and could not easily mock the object that returns the peeraddr as its inside the reactor class.

Possible reasons why this error could occur? https://github.com/ruby/ruby/blob/ddf5020e4fcae5ed28a064af10124a032590452f/ext/socket/socket.c#L366

  • Errno::EINVAL - the address length used for the sockaddr is not a valid
  • Errno::EINVAL - the +socket+ is already bound to an address, and the
  • protocol does not support binding to the new sockaddr or the socket has been shut down.
  • Errno::EINVAL - the address length is not a valid length for the address family

My idea was to create a test that has an expired SSL, that means we hit this part of the code

rescue MiniSSL::SSLError => e
then have the peeraddr throw from a mocked object, but could not seem to do that.

Any idea on how I can do this, or is there another way I can go about writing a test for this?

@hahmed hahmed changed the title Fixe peeraddr raises Errno::EINVAL inside reactor Fix peeraddr raises Errno::EINVAL inside reactor Oct 6, 2019
@nateberkopec
Copy link
Member

Yikes. I got about neck-deep into this one before I realized we would need a ground-up redesign of Reactor and Client to really stub this correctly, or give up and use any-instance mocks.

Merging without a test, under great pain.

@nateberkopec
Copy link
Member

Closed w 70e381d

Thanks for your 👀 @hahmed, this one turned out to be a lot more complicated to test than almost anything we've looked at yet...

@hahmed
Copy link
Contributor Author

hahmed commented Oct 7, 2019

Thanks, yeah I tried everything to find a way to test this, at least I learned a lot about how the reactor works 😂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants