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

Connection Options for em-httprequest #88

Closed
rorra opened this issue Mar 24, 2013 · 8 comments
Closed

Connection Options for em-httprequest #88

rorra opened this issue Mar 24, 2013 · 8 comments
Assignees
Labels
Milestone

Comments

@rorra
Copy link

rorra commented Mar 24, 2013

If I'm right, on Savon, to create a connection with timeout options, you do something like

client = Savon.client(wsdl: wsdl_content, open_timeout: 60, read_timeout: 120)

but on em-httprequest (version 1.0.3), the class HttpConnectionOptions, which has a constructor where it expects the uri and the options, never gets the options (which if I'm right, should be :connect_timeout and :inactivity_timeout).

later when I call an operation, with:

client.call(:my_operation, message: my_message)

on Savon::Operation.call! the request is created by using HTTPI, and when it tries to do the request, on:

HTTPI::request, the line of code:

adapter_class = load_adapter(adapter, request)

loads the adapter HTTPI::Adapter::EmHttpRequest

but when I check the adapter connection, I get:

(rdb:1) adapter_class.client
<EventMachine::HttpConnection:0x007fc2cb538208 @deferred=true, @middleware=[], @connopts=#<HttpConnectionOptions:0x007fc2cb538e88 @connect_timeout=5, @inactivity_timeout=10, @tls={}, @proxy=nil, @host="my_host.com", @port=443>, @uri="https://my_url.com/my_path">

so you can see that the connection_timeout is 5, and the inactivity_timeout is 10, and the HTTPI::Request that is used to build the EventMachine::HttpConnection is:

#<HTTPI::Request:0x007fc2cab23fd8
 @body=
  "MY_BODY_REQUEST",
 @headers=
  {"MY_HEADER"},
 @open_timeout=60,
 @read_timeout=120,
 @url=#<URI::HTTPS:0x007fc2ca84f7a8 URL:https://my_url.com/my_path>>

in few words, I cannot use timeout options with em-httprequest

@rorra
Copy link
Author

rorra commented Mar 24, 2013

So with the request:

<HTTPI::Request:0x007f9368dd5268
 @body=
  "MY_BODY",
 @headers=
  {"MY_HEADERS},
 @open_timeout=60,
 @read_timeout=120,
 @url=MY_URL>

HTTPI::Adapter::EmHttpRequest is constructed, which calls:

@client = EventMachine::HttpRequest.new build_request_url(request.url)

But EventMachine::HttpRequest constructor is:

    def self.new(uri, options={})
      connopt = HttpConnectionOptions.new(uri, options)

      c = HttpConnection.new
      c.connopts = connopt
      c.uri = uri
      c
    end

so the HttpConnectionOptions never gets the options for the timeout, it only get the url, so it ends using the default options on the class HttpConnectionOptions, which includes a default connect timeout of 5 seconds and a inactivity timeout of 10 seconds.

@rorra
Copy link
Author

rorra commented Mar 24, 2013

Another thing, is that I saw that the options for timeout are passed when the request is made, on HTTPI::Adapter.request:

      def request(method)
        _request { |options| @client.send method, options }
      end

that will hit EventMachine::HTTPMethods.setup_request, which uses the options and pass it to the constructor of HttpClientOptions, but this constructor expects options like:

  • keepalive
  • redirect
  • followed
  • head
  • query
  • path
  • file
  • body
  • pass_cookies
  • decoding

No way to send the timeout flags at this time, so definetly they should be sent when the EventMachine::HttpRequest is created.

A monkey patch:

module HTTPI
  module Adapter
    class EmHttpRequest < Base
      def initialize(request)
        @request = request
        @client = EventMachine::HttpRequest.new(
            build_request_url(request.url),
            {connect_timeout: request.open_timeout, inactivity_timeout: request.read_timeout}
        )
      end
    end
  end
end

if you let me know if that code can actually be merged, and that it make sense, then I'll work on the rspec and do a pull request.

@rubiii
Copy link
Contributor

rubiii commented Mar 24, 2013

thanks @rorra. if it's backed by specs, i think we should get your fix in.
@rogerleite what do you think?

@rogerleite
Copy link
Member

@rorra thanks for all the info you put here!
@rubiii I think we can go with @rorra solution, passing options to client initialize. Like the monkey patch, with a tiny change:

    def initialize(request)
      @request = request
      @client = EventMachine::HttpRequest.new(
          build_request_url(request.url),
          self.client_options
      )
   end

Go ahead with the pull request! 😄

@rogerleite
Copy link
Member

Looking to the sources, only HttpConnectionOptions accept proxy option, so, i think we have the same problem with proxy option.

@rorra
Copy link
Author

rorra commented Mar 26, 2013

I think that the problem by using self.client_options, is that the options that em-httprequest expects are not the same options that httpi has, that's the reason I did the transformation inline:

{connect_timeout: request.open_timeout, inactivity_timeout: request.read_timeout}

httpi has open_timeout and read_timeout, while em-httprequest has connect_timeout and inactivity_timeout, and I bet its probably the same with other options like the proxy.

I'll work on the rspec and see if I'm able to add the proxy options as well and ask for a review once I'm done.

@rogerleite
Copy link
Member

@rorra The client_options is a private method on em_http adapter that "converts" httpi request options to em_http options.

You could create a private method, something like connection_options, to just have timeout and proxy options, and use that on initialize. It would be more clear, i agree.

If you have question or need any tip, just let us know!

@rubiii
Copy link
Contributor

rubiii commented Jul 22, 2013

released with version 2.1.0.

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

No branches or pull requests

3 participants