get rid of global state

[rubiii]

it prevents e.g. using multiple loggers for HTTPI in one project.
i would like to see all methods on the HTTPI module which deal with global state gone.
i already did this for several savonrb libraries including nori for the savon 2.0 release.

here's the list of methods that come to mind:

• HTTPI.adapter=
• HTTPI.log=
• HTTPI.logger=
• HTTPI.log_level=

maybe, if we changed the HTTPI module to a class, we could create HTTPI instances which hold these options. the static http request method on the new HTTPI class like .post could then be changed to shortcuts for creating a new instance with options and executing a request.

don't know if that's the best idea though.

[rogerleite]

Actually, i don't see any problem about HTTPI's global state.
Maybe if you put some code examples, i can get what you're saying.

I'm planning to refactor Adapter module to class, and maintain adapter instances to be reused in request calls (and close feature #41).

[rubiii]

well, the problem with global state is that you can't for example use multiple different loggers for httpi in a single project. let's say you're using savon to connect to two different services and want to log to different files. that's not possible right now without changing global state, which does not work too well in threaded environments.

also, if you're using httpi via some other project (like savon or http_monkey), users of these project have to know about httpi to set a default http client or change the logger. it's just not easily possible to provide these options in libraries using httpi.

i'm not sure about what to do about the default adapter option, but i think it would be ok to remove it in a future version and have httpi figure out which adapter to use. i think it already does that. and if it would be possible to create an httpi instance of some kind, the default adapter, logging, etc could be set on the instance instead.

[minad]

Ok, so what I would suggest (I already wrote this on #77, but I misunderstood some things there because I wasn't deep enough in the code).

I would create client objects like this

client = HTTPI.new
client.get(...)
...
client.close

Then one wouldn't have a global state and could support persistent connections.

One could still provide convenience methods HTTPI.get etc which have the overhead that they always create a new HTTPI instance each time, so the current api would still exist more or less. But I would remove the possibility to select different backends per call. I don't know why this is useful.

Another idea is that you could apply a rack-like concept where you create proxies/middleware clients. Each middleware should implement a request method for example. One could create a client which handle for example cookies. The underlying client deals only with http headers, the client middleware stores them. I applied something similar to Moneta: https://github.com/minad/moneta

[rubiii]

changing the design could make it easier to support persistent connections, but it's only a very small part of a larger issue. ps. if you need middleware, you should probably use faraday.

[rogerleite]

if you need middleware, i recommend http_monkey too.

[rubiii]

right 😄

[rogerleite]

Exploring @minad idea, we could:

adapter = HTTPI.new  # default net/http or HTTPI.new(:curb)
# adapter can have some options from Request object.
# all changes on this "adapter attributes", should apply on native adapter immediately
adapter.proxy = "http://proxy.com"

# adapter have verbs
response = adapter.get("http://url.com")
# we would need to split "adapter attributes" from "request options"
response = adapter.post("http:/url.com", "body")

adapter.close

It's a big work, specially separating adapter options from request options.
And i'm not thinking how to support "old" interface.

[rubiii]

what exactly do you mean by "splitting adapter attributes from request options"?
in the end, all request options need to be passed to an adapter somehow.
still not sure if the benefits outweigh the costs.

[rogerleite]

@rubiii i agree with you. The connection_id idea is easier than this and has the same effect.

[ioquatix]

The design suggested by @rogerleite makes way more sense. But the reality is, I'm not sure if anyone is going to try and (1) basically re-implement this gem from the ground up and (2) refactor all existing places when (3) a gem like Faraday exists which already does 99% of what you have above.

Rather than reinventing the wheel, I'd suggest we aim to gracefully deprecate this gem, and rewrite the places where it's used either with an abstract interface with dependency injection or directly rely on something like faraday (or equivalent).

[pcai]

I second this suggestion from @ioquatix to soft-deprecate this gem, rather than try to "get rid of global state" - compiling some reasons:

• as already mentioned, such a change would make it almost identical in form and function to faraday, so its not clear what the community gains from having 2 duplicative ways to solve the same problem
• maintenance bandwidth for this gem and other savon projects is already very minimal, and thus very precious. committing to a rewrite means spending significant effort to update not only this gem, but many dependents

So this translates roughly into:

• close all issues tracking threadsafety/state problems that would need major new features or rewrites to accomplish
• update documentation to reflect that the gem is in maintenance, and likely won't be releasing or accepting new features moving forward (we can take anything in progress on a case by case basis)
• publish a migration guide to something like faraday?

I am open to alternative ideas, but at this point it would require someone to be extremely persuasive and simultaneously generous with their time for it to be reasonable

[pcai]

Closing this as a follow-up to my prior message - please see also #238. thanks