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

Make HTTP::Feature work like rack middleware #767

Closed
souk4711 opened this issue Oct 6, 2023 · 10 comments · Fixed by #766
Closed

Make HTTP::Feature work like rack middleware #767

souk4711 opened this issue Oct 6, 2023 · 10 comments · Fixed by #766

Comments

@souk4711
Copy link
Contributor

souk4711 commented Oct 6, 2023

I am working on http-session (almost done) to support cookies and caching. I found that the HTTP::Feature does not work like the Rack middleware, e.g. executing orders:

require "http"

cls = Class.new(HTTP::Feature) do
  @orders = []

  def self.orders
    @orders
  end

  def initialize(id:)
    @id = id
  end

  def wrap_request(req)
    self.class.orders << "req.#{@id}"
    req
  end

  def wrap_response(res)
    self.class.orders << "res.#{@id}"
    res
  end

  HTTP::Options.register_feature(:feature_orders_1, self)
  HTTP::Options.register_feature(:feature_orders_2, self)
end

sub = HTTP.use(
  feature_orders_1: {id: 1},
  feature_orders_2: {id: 2}
)
sub.get("https://httpbin.org/cache/0") 


# Expect: ["req.1", "req.2", "res.2", "res.1"]
# Actual: ["req.1", "req.2", "res.1", "res.2"]
p cls.orders

The PR#766 attempts to address this and make HTTP::Feature work more like Rack middleware, so that developers can more easily implement their own features, like auth, retry, cache, etc.

Note that this PR may break some existing code that depends on orders, and replay function in webmock.

@tarcieri
Copy link
Member

tarcieri commented Oct 6, 2023

This was a deliberate design decision.

See the many, many years of previous discussion. Here's a summary: #478 (comment)

This was the resolution: #482 #499

@souk4711
Copy link
Contributor Author

souk4711 commented Oct 6, 2023

This was a deliberate design decision.

See the many, many years of previous discussion. Here's a summary: #478 (comment)

This was the resolution: #482 #499

I respect your decision. But is it possible to change the execution orders?

Just change client.rb#L84:

# res = options.features.inject(res) do |response, (_name, feature)|
res = options.features.to_a.reverse.to_h.inject(res) do |response, (_name, feature)|
  feature.wrap_response(response)
end

Suppose we have three features, logging, auto_deflate, auto_inflate. If i want log the uncompressed request/response body, only [:auto_inflate, :logging, :auto_deflate] make sense. It's a little strange.

[:logging, :auto_deflate, :auto_inflate] # response in log is compressed
[:logging, :auto_inflate, :auto_deflate] # response in log is compressed
# 1. logging#wrap_request
# 2. auto_deflate#wrap_request
# 3. _REAL_HTTP_REQUEST_
# 4. logging#wrap_response
# 5. auto_inflate#wrap_response

[:auto_deflate, :auto_inflate, :logging]  # request in log is compressed
[:auto_inflate, :auto_deflate, :logging]  # request in log is compressed
# 1. auto_deflate#wrap_request
# 2. logging#wrap_request
# 3. _REAL_HTTP_REQUEST_
# 4. auto_inflate#wrap_response
# 5. logging#wrap_response

[:auto_deflate, :logging, :auto_inflate] # request/response in log is compressed
# 1. auto_deflate#wrap_request
# 2. logging#wrap_request
# 3. _REAL_HTTP_REQUEST_
# 4. logging#wrap_response
# 5. auto_inflate#wrap_response

@tarcieri
Copy link
Member

tarcieri commented Oct 6, 2023

If you're asking if it would be okay for features to be applied to responses to handled in the reverse order from the requests, that makes sense to me, though I would worry somewhat about it breaking existing code if it were done outside of a breaking release.

@souk4711
Copy link
Contributor Author

souk4711 commented Oct 6, 2023

If you're asking if it would be okay for features to be applied to responses to handled in the reverse order from the requests, that makes sense to me, though I would worry somewhat about it breaking existing code if it were done outside of a breaking release.

Updated, plz see #766

@ixti
Copy link
Member

ixti commented Oct 14, 2023

Oh. I really like that you've started working on session gem. I was thinking about it for a very long time (and due to some personal issues never had time for that). I was thinkign about a bit different API for that though:

session = HTTP::Session.new(...)

session.with do |http|
  http.headers(...).get(...)
end

The idea I had was to use connection pool, so that connection would be kept alive just like in browsers.

@ixti
Copy link
Member

ixti commented Oct 14, 2023

On the features order of execution — I think we should provide them with the expected order of the execution. In rack middlewares world, you can tell where to inject the middleware. We should come with similar solution, so that features will be executed in predictable order.

HTTP.use(:feature_1).use(:feature_2, before: :feature_1)

Or using "weights" on features themselves (this will be less predicatble though):

class AutoInflateFeature < Feature
  weight 9000

  ...

end


class LoggingFeature < Feature
  weight 0
end

Weight-based one will allow us to change use API as well to allow override default weight:

HTTP.use(:logging, weight: 100)

NB: I'm not proposing going russian-doll middleware pattern. But to provide a way to control feature execution order.

@tarcieri
Copy link
Member

However that ordering is determined, I think reversing that ordering for processing responses as in #766 better maps to the sort of LIFO/FILO ordering you'd get with middlewares, where the ordering for middlewares lives in the call stack

@ixti
Copy link
Member

ixti commented Oct 14, 2023

However that ordering is determined, I think reversing that ordering for processing responses as in #766 better maps to the sort of LIFO/FILO ordering you'd get with middlewares, where the ordering for middlewares lives in the call stack

I totally agree with that.

@ixti ixti closed this as completed in #766 Oct 14, 2023
@souk4711
Copy link
Contributor Author

souk4711 commented Oct 14, 2023

Oh. I really like that you've started working on session gem. I was thinking about it for a very long time (and due to some personal issues never had time for that). I was thinkign about a bit different API for that though:

session = HTTP::Session.new(...)

session.with do |http|
  http.headers(...).get(...)
end

The idea I had was to use connection pool, so that connection would be kept alive just like in browsers.

HTTP::Session has a similar api interface to HTTP::Client, such as timeout/follow/via, except persistent.

IMO, we can turn on the keep-alive feature for all requests when using session, like python-requests. e.g.

http = HTTP::Session.new(..., persistent: true).timeout(8).freeze

http.get('https://example.com/abc') # -> make a persistent connection
http.get('https://example.com/def') # -> automatically reuse the connection

But in order to work with multiple threads, a bit more work is required. So it not implement yet.

@souk4711
Copy link
Contributor Author

souk4711 commented Oct 14, 2023

On the features order of execution — I think we should provide them with the expected order of the execution. In rack middlewares world, you can tell where to inject the middleware. We should come with similar solution, so that features will be executed in predictable order.

HTTP.use(:feature_1).use(:feature_2, before: :feature_1)

Or using "weights" on features themselves (this will be less predicatble though):

class AutoInflateFeature < Feature
  weight 9000

  ...

end


class LoggingFeature < Feature
  weight 0
end

Weight-based one will allow us to change use API as well to allow override default weight:

HTTP.use(:logging, weight: 100)

NB: I'm not proposing going russian-doll middleware pattern. But to provide a way to control feature execution order.

We may still need a way to adjust execution orders, e.g.

http = HTTP.use(:feature_1)

http.get('https://example.com/abc')
http.get('https://example.com/def', features: { feature_2: {} }) # only want to use this feature in this request, and want it to run before :feature_1

But currently it may not necessary.

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

Successfully merging a pull request may close this issue.

3 participants