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

Installing Twitter Gem breaks Savon due to HTTPI LOAD_ORDER preferring http over net_http #156

Closed
lustremedia opened this issue Jul 29, 2015 · 16 comments

Comments

@lustremedia
Copy link

Using savon (2.11.1)
Using httpi (2.4.1)
Using twitter (5.14.0)
Using http (0.6.4)

I just installed the twitter Gem (https://github.com/sferik/twitter) which has http Gem as dependency while using the savon Gem .

HTTPI then preferred http over the working net_http (this seems to be the default in Rails) in the LOAD_ORDER which then breaks Savon's requests with following error:
NoMethodError (undefined method headers' for #HTTP::Client:0xbd8ab91c):`

After installing httpclient everything worked fine again due to the LOAD_ORDER
https://github.com/savonrb/httpi/blob/master/lib/httpi/adapter.rb#L16

This seems to be related to savonrb/savon#488

The error messages are not very clear and the stacktrace for debugging was really non-existing but pointing to the Savon API calls. I think this ought to be remedied in some way.

@lustremedia lustremedia changed the title Installing Twitter Gem breaks Savon due to HTTPI LoadOrder preferring http over net_http Installing Twitter Gem breaks Savon due to HTTPI LOAD_ORDER preferring http over net_http Jul 29, 2015
@lustremedia
Copy link
Author

Also most probably relates to #16 which would have been a time saver if the right error message would have been thrown!

@mikecmpbll
Copy link

relates more to #148. You can change the adapter that Savon uses though, as explained in comments on other issue:

client = Savon.client(
      ...
      :adapter => :net_http
    )

Seems as though this headers issue is an issue with the 'http' adapter; not sure what version of the 'http' gem @Connorhd developed the adapter against?

@lustremedia
Copy link
Author

@mikecmpbll what is the recommended adapter? Couldn't the team of savon just select one as dependency and be done with it? It certainly is the abstraction that caused the error but I am sure there is a reason for that.

The time consuming part was making sense of the error message and the close to "non-existing stacktrace" about what caused the issue and from what gem it stemmed from.

and yeah #148 seems to be the issue, but not under the update condition. However savonrb/savon#488 showed me the way on how to resolve it and how to make sense of it all.

@mikecmpbll
Copy link

@lustremedia the LOAD_ORDER that you mentioned specifies HTTPI's adapter preference order, however you can use whichever adapter works best for you. HTTPI is a single interface to the myriad of http adapters available. The reason for your error appears to be an issue with the adapter for the http.rb gem, but I haven't had time yet to look in to it.

The error almost certainly stems from this line of the adapter, though: https://github.com/savonrb/httpi/blob/master/lib/httpi/adapter/http.rb#L72 so it should be fairly easy for someone to work out what's up.

The update itself isn't a condition in #148–the conditions are version 2.4.1 of httpi which has the http.rb support, and having the http gem installed.

FWIW, I understand that Savon 3 will be dropping the HTTPI dependency and leaving it up to Savon end-users to extend their favourite HTTP library.

@lustremedia
Copy link
Author

@mikecmpbll As you could see from my incident description, I resolved the issue but wanted to let you guys know about. I searched the web and even this repo but #148 did not come up for me.

Documentation about :adapter switch in savon is also nowhere to be found. Thanks for taking your time however to point out more about the issue and dropping the HTTPI dependency would be less headaches in the future I think.

Cheers

@rogerleite
Copy link
Member

Hi @lustremedia. Sorry for the trouble. I don't know what httpi can do to help on these cases. Maybe savon could log what adapter is using or something like this.

@mikeantonelli thanks for helping! 👍

@mikecmpbll
Copy link

@lustremedia thanks for reporting it, definitely something here that needs to be fixed so I'll see if I can check it out this evening.

and yep, for some reason the adapter option is not documented for Savon..

@lustremedia
Copy link
Author

Hi @rogerleite As I suggested: pick a single production ready adapter for savon and ship it with savon as dependency, but keep the option to change the adapter in the client if the developer wants something different.

In that case, a well tested adapter is always present as default. If the developer chooses to use a different adapter but the adapter is broken he/she can report a bug with the adapter and savon would not be affected, but could point to the default adapter.

@mikecmpbll 👍 Thanks for the help!

And thank you both for the great and fast responses! 👍

@lustremedia
Copy link
Author

@rogerleite also you could have the option to reject support for less active adapters and have less troubles.

@rogerleite
Copy link
Member

@lustremedia i agree with you. Seeing savonrb/savon#488, this suggestion can avoid a lot of problem, you can open an issue on savon.

Thanks for reporting! I'm copying @tjarratt to be aware of this.

@lustremedia
Copy link
Author

@rogerleite see savonrb/savon#710 cheers

@Connorhd
Copy link
Contributor

Sorry for causing pain here since its my adapter thats not working, the problem is you have an older version of the http gem. If you update to a more recent version it should work.

@lustremedia
Copy link
Author

@Connorhd np! See, if I get the newest http gem it might break the twitter gem since the twitter gem has the http gem as dependency locked at a certain version which seems to work for them. Maybe they have already updated, but this triggered the savon LOAD_ORDER and broke Savon ...

@Connorhd
Copy link
Contributor

It looks like the next version of the twitter gem uses newer http, obviously that doesn't help you now :(

I wonder if httpi should allow adapters to specify extra requirements to be enabled, so a minimum version could be given.

@lustremedia
Copy link
Author

@Connorhd I think if part of your code plays such an important role as to be a showstopper, maybe it is time for savon to decide on one http client and be done with it, rather than supporting all http clients and trying to figure out a LOAD_ORDER of the least client that is going to break.

@mikecmpbll
Copy link

That's a discussion for over at savon. As far as HTTPI is concerned, I agree that adapters should be able to specify version dependency for their respective libraries.

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

No branches or pull requests

4 participants