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

Change to HTTP gem dependency breaks Twitter::Streaming::Connection v6 #709

Closed
davebell opened this issue Jul 20, 2015 · 12 comments
Closed
Assignees

Comments

@davebell
Copy link

The HTTP (gem) dependency has changed from ~> 0.6.0 for v5.14.0 to ~> 0.8.3 for v6.0.0.

There's been a change to how the HTTP gem creates request object URIs. The former uses <URI> while the latter uses <HTTP::URI> which is a subclass of <Addressable::URI>:

0.6.x

@uri    = uri.is_a?(URI) ? uri : URI(uri.to_s)

0.8.x

@uri    = HTTP::URI.parse(uri).normalize

The 0.8.x HTTP::URI.parse method does not set the port based on the uri scheme:

0.6.x

[38] pry(main)> uri = URI('https://stream.twitter.com')
=> #<URI::HTTPS:0x007fa54f8d38e8 URL:https://stream.twitter.com>
[39] pry(main)> uri.port
=> 443

0.8.x

[40] pry(main)> uri = HTTP::URI.parse('https://stream.twitter.com').normalize
=> #<Addressable::URI:0x3fd2a57bd650 URI:https://stream.twitter.com/>
[41] pry(main)> uri.port
=> nil

As a result, when creating a Twitter::Streaming::Connection with 0.8.x, the TCPSocket is passed nil for the port argument.

module Twitter
  module Streaming
    class Connection
      attr_reader :tcp_socket_class, :ssl_socket_class

      def initialize(options = {})
        @tcp_socket_class = options.fetch(:tcp_socket_class) { TCPSocket }
        @ssl_socket_class = options.fetch(:ssl_socket_class) { OpenSSL::SSL::SSLSocket }
      end

      def stream(request, response)
        client_context = OpenSSL::SSL::SSLContext.new
        client         = @tcp_socket_class.new(Resolv.getaddress(request.uri.host), request.uri.port) #<-- Port is not being set
        ssl_client     = @ssl_socket_class.new(client, client_context)

        ssl_client.connect
        request.stream(ssl_client)
        while body = ssl_client.readpartial(1024) # rubocop:disable AssignmentInCondition, WhileUntilModifier
          response << body
        end
      end
    end
  end
end

This results in either a failed connection on OS X or a connection timeout on Linux.

OS X 10.10.4

Errno::EADDRNOTAVAIL: Can't assign requested address - connect(2) for "199.16.156.217" port 
from /ruby/2.1.4/lib/ruby/gems/2.1.0/bundler/gems/twitter-a35bf7cc6b69/lib/twitter/streaming/connection.rb:17:in `initialize'

Ubuntu 14.04

Errno::ETIMEDOUT: Connection timed out - connect(2) for "199.16.156.20" port 
from /ruby/2.1.4/bundler/gems/twitter-a35bf7cc6b69/lib/twitter/streaming/connection.rb:17:in `initialize'
stve added a commit that referenced this issue Jul 24, 2015
@stve
Copy link
Collaborator

stve commented Jul 24, 2015

hi @davebell, thanks for reporting this. I've fixed this in the streaming-updates branch

@stve stve self-assigned this Sep 7, 2015
@davidcelis
Copy link

Hey @stve, I'm experiencing this problem. Is there any word on getting the streaming-updates branch merged into a release? Or is there a workaround in the meantime?

@stve
Copy link
Collaborator

stve commented Sep 14, 2015

@davidcelis what branch are you running against? or are you using v5 of the gem?

The streaming-updates branch is targeted for v6 and I made good progress on some of the goals I had for streaming for that release but have gotten busy and haven't been able to work on it in a while. To get a better idea of what is going to be included, check #631

@dentarg
Copy link

dentarg commented Nov 12, 2015

Was cfa146c related to this issue?

@jbonhag
Copy link

jbonhag commented Nov 15, 2015

A quick-n-dirty fix is to hardcode the port on line 16 of twitter-5.15.0/lib/twitter/streaming/connection.rb, a la:

client         = @tcp_socket_class.new(Resolv.getaddress(request.uri.host), 443)

@betandr
Copy link

betandr commented Nov 18, 2015

Thanks @jeffbonhag, that works for twitter-5.15.0 👍

@barrettclark
Copy link

Another [really gross] workaround is to just monkey-patch the HTTP gem:

# Monkey-patch HTTP::URI                                                                                                                                                                                
class HTTP::URI                                                                                                                                                                                         
  def port                                                                                                                                                                                              
    443 if self.https?                                                                                                                                                                                  
  end                                                                                                                                                                                                   
end

@magedmakled
Copy link

👍 @jeffbonhag Worked for me too in twitter-5.15.0

@Anachron
Copy link

Cannot believe this is still open, I just ran into it.

Edit: The fix from @jeffbonhag seems to be working, but I don't really like monkey-patching.

@davidcelis
Copy link

Yeah this is kind of a serious bug to still be present. I'd like to think that a patch, even the above workaround, could be implemented as a bug fix in the meantime.

@daturkel
Copy link

This commit fixes it, but I don't believe it's been merged? https://github.com/animeavi/twitter/commit/6d08c5932d3033d5b69537730e02ac5905775410

@sferik
Copy link
Owner

sferik commented Jan 19, 2016

Sorry it took so long to fix this. I’m planning to push a new release including this fix tomorrow.

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

No branches or pull requests