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

Move read_buffer guards in Socket #797

Merged
merged 5 commits into from Nov 8, 2022

Conversation

vasi-stripe
Copy link
Contributor

Socket#read_nonblock currently has a guard around selecting on the socket in the HTTP case. This makes sure that if we have any data, we don't block waiting for more, but just return it right away.

However, the HTTPS case does not include such a guard. This PR proves that this is mistaken, but running streaming tests in SSL mode, and showing that they fail without the guard, but pass once it's added.

We also remove the superfluous guard in read_block, since it doesn't make sense. In blocking mode, @read_buffer is unused and will always be empty—checking if it's empty is harmless, but complicates the code and achieves nothing.

Best reviewed commit-by-commit:

  • Commits 1-2 prepare for running streaming tests in multiple modes (HTTP/HTTPS), by factoring out these tests into a method, and moving it to test_helper.rb
  • Commit 3 adds a small bespoke SSL streaming server, and runs the streaming tests against it
    • I couldn't find any simpler way to run an SSL streaming server, other than writing it myself. WEBrick doesn't like streaming, Unicorn doesn't do SSL without a proxy, Thin has a weird streaming model that I don't understand. Ideas welcome!
    • Here's the test output we get as of this commit. Note the SSL streaming tests fail, because once we've read "Hello" we get a "would block" error and wait for more data—instead of returning what we have right away, as a non-block socket should.
  • Commit 4 fixes the above test failures, by adding the guard. Now if we have some data, we return it instead of waiting.
  • Commit 5 removes the superfluous guard in read_block, since @read_buffer will always be empty in blocking mode

We'll move these to test_helper.rb soon, so we can test both HTTP
and HTTPS
Notice a couple of tests fail now
Allows SSL streaming tests to pass. Without this, we wait even though
we already have some data to return.
This has no effect, since @read_buffer is completely unused in
blocking mode. It just keeps the code cleaner.
Copy link
Contributor

@geemus geemus left a comment

Choose a reason for hiding this comment

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

Thanks for your help and all the details, definitely makes it easier.

I'd love to say I know of an easier or cleaner way to setup for tests, but I don't off hand. Getting servers to behave and/or misbehave in the ways we want for tests has been one of the bigger challenges at times with all of this, so we usually have just done whatever we could figure out that would work. Maybe we will revisit that at some point to simplify and streamline in some way, but what you've done seems fine for now.

@geemus geemus merged commit 0a8b9e5 into excon:master Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants