Skip to content

request: in the event of failure, try other IPs (#6761)#6813

Merged
Gargron merged 6 commits intomastodon:masterfrom
vulpineclub:fix-6761
Mar 20, 2018
Merged

request: in the event of failure, try other IPs (#6761)#6813
Gargron merged 6 commits intomastodon:masterfrom
vulpineclub:fix-6761

Conversation

@rtucker
Copy link
Copy Markdown
Contributor

@rtucker rtucker commented Mar 18, 2018

In the case where a name has multiple A/AAAA records, we should
try subsequent records instead of immediately failing when we have a
failure on the first IP address.

This significantly improves delivery success when there are network
connectivity problems affecting only IPv4 or IPv6.

Rey Tucker added 2 commits March 17, 2018 21:33
@rtucker
request: in the event of failure, try other IPs (mastodon#6761)
6de5868 
In the case where a name has multiple A/AAAA records, we should
try subsequent records instead of immediately failing when we have a
failure on the first IP address.

This significantly improves delivery success when there are network
connectivity problems affecting only IPv4 or IPv6.
@rtucker
fix method call style
515da76
@rtucker
Copy link
Copy Markdown
Contributor Author

rtucker commented Mar 18, 2018 •
edited
Loading

The CI build is failing on:

rspec ./spec/lib/request_spec.rb:63 # Request#perform with private host raises Mastodon::ValidationError

This is due to the test case being tied to IPSocket.getaddress. I don't currently have a good development environment for running the tests, so feel free to fix this up. (NOTE: I can now run tests)

Also there ought to be a test case to test for the round-robin working as expected.

akihikodaki
akihikodaki previously requested changes Mar 18, 2018
View reviewed changes
app/lib/request.rb Outdated
super address, *args
outer_e = nil
Addrinfo.foreach(host, nil, nil, :SOCK_STREAM) do |address|
raise Mastodon::HostValidationError if PrivateAddressCheck.private_address? IPAddr.new(address.ip_address)
Copy link
Copy Markdown
Contributor

@akihikodaki akihikodaki Mar 18, 2018

Choose a reason for hiding this comment

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

Please move this line in the begin...rescue section. Such a case that one of addresses are invalid but the others are valid should have been tolerated in releases prior to v2.3.0.

@akihikodaki akihikodaki added the bug Something isn't working label Mar 18, 2018
@akihikodaki akihikodaki dismissed their stale review March 18, 2018 03:23

Now the spec is the only remanining.

@rtucker
Copy link
Copy Markdown
Contributor Author

rtucker commented Mar 18, 2018

Currently stalled on:

/home/travis/build/tootsuite/mastodon/vendor/bundle/ruby/2.4.0/gems/http-3.0.0/lib/http/connection.rb:126:in `close': HTTP::Timeout::Null#closed? at /home/travis/.rvm/rubies/ruby-2.4.2/lib/ruby/2.4.0/forwardable.rb:157 forwarding to private method NilClass#closed?
                                                                                
  1) Request#perform with private host raises Mastodon::ValidationError
     Failure/Error: expect{ subject.perform }.to raise_error Mastodon::ValidationError
     
       expected Mastodon::ValidationError, got #<NoMethodError: undefined method `closed?' for nil:NilClass on http://example.com/> with backtrace:
         # /home/travis/.rvm/rubies/ruby-2.4.2/lib/ruby/2.4.0/forwardable.rb:227:in `closed?'
         # 
         #   Showing full backtrace because every line was filtered out.
         #   See docs for RSpec::Configuration#backtrace_exclusion_patterns and
         #   RSpec::Configuration#backtrace_inclusion_patterns for more information.
     # ./spec/lib/request_spec.rb:74:in `block (4 levels) in <top (required)>'
     # ./spec/lib/request_spec.rb:66:in `block (4 levels) in <top (required)>'

My Ruby/RSpec experience is lacking, so I'm going to take a break for the night.

akihikodaki
akihikodaki suggested changes Mar 18, 2018
View reviewed changes
spec/lib/request_spec.rb Outdated
end

it 'executes a HTTP request when the first address is private' do
allow(Addrinfo).to receive(:foreach).with('example.com', nil, nil, :SOCK_STREAM).and_return([
Copy link
Copy Markdown
Contributor

@akihikodaki akihikodaki Mar 18, 2018

Choose a reason for hiding this comment

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

See https://relishapp.com/rspec/rspec-mocks/docs/configuring-responses/yielding.

Gargron
Gargron reviewed Mar 18, 2018
View reviewed changes
app/lib/request.rb
@@ -94,9 +94,16 @@ def http_client
class Socket < TCPSocket
Copy link
Copy Markdown
Member

@Gargron Gargron Mar 18, 2018

Choose a reason for hiding this comment

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

@akihikodaki Isn't there different classes for Socket and SSLSocket?

Copy link
Copy Markdown
Contributor

@akihikodaki akihikodaki Mar 18, 2018

Choose a reason for hiding this comment

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

SSLSocket uses TCPSocket, so patching only TCPSocket is enough.

Copy link
Copy Markdown
Member

@Gargron Gargron Mar 18, 2018

Choose a reason for hiding this comment

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

But this doesn't patch TCPSocket, only our own Socket class, no?

Copy link
Copy Markdown
Contributor

@akihikodaki akihikodaki Mar 18, 2018

Choose a reason for hiding this comment

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

SSLSocket takes a TCPSocket instance as an argument when initializes, and HTTP gem uses our patched TCPSocket to create an instance for the argument. That's how it works.

Copy link
Copy Markdown
Member

@Gargron Gargron Mar 18, 2018

Choose a reason for hiding this comment

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

Okay, thanks.

akihikodaki
akihikodaki approved these changes Mar 20, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@akihikodaki akihikodaki left a comment

Choose a reason for hiding this comment

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

I fixed the spec, and now it is ready to merge. I hope it will be in v2.3.2.

@Gargron Gargron merged commit 36b5703 into mastodon:master Mar 20, 2018
smorimoto pushed a commit to kibousoft/mastodon that referenced this pull request Apr 26, 2018
@rtucker @Gargron
request: in the event of failure, try other IPs (mastodon#6761) (mast…
178fa4a 
…odon#6813)

* request: in the event of failure, try other IPs (mastodon#6761)

In the case where a name has multiple A/AAAA records, we should
try subsequent records instead of immediately failing when we have a
failure on the first IP address.

This significantly improves delivery success when there are network
connectivity problems affecting only IPv4 or IPv6.

* fix method call style

* request_spec: adjust test case to use Addrinfo

* request: Request/open: move private addr check to within begin/rescue

* request_spec: add case to test failover, fix exception check

* Double Addrinfo.foreach so that it correctly yields instances
@Gargron Gargron mentioned this pull request Nov 22, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Gargron Gargron Gargron left review comments

+1 more reviewer

@akihikodaki akihikodaki akihikodaki approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rtucker @Gargron @akihikodaki