postrank-labs / goliath

Goliath is a non-blocking Ruby web server framework
Other
2.44k stars 217 forks source link

The response part in all the responses should respond to "each" as per Rack spec #239

Open kgrz opened 11 years ago

kgrz commented 11 years ago

In almost all the examples, the response method is something like:

def response(env)
  [200, {}, "some string"]
end

As per Rack spec, the third element in the response array should respond to each which was true for a String object in Ruby 1.8. Since 1.9 though, this is not the case. So, should the examples be updated accordingly?

kgrz commented 11 years ago

I might add why this was an issue:

I have two apps: 1 Goliath service that returns a json after processing the request and 1 Sinatra web app that talks to this service.

When the Sinatra app was deployed with Thin (development mode), there was no issue when I returned a JSON string similar to {'test': 'Works'}. But when the app was deployed using Unicorn, it broke the app raising a Rack::Lint error. Returning a response of [{'test': 'Works'}] fixes it.

dj2 commented 11 years ago

This was one of the reasons why we don't claim we're fully rack compliant. There were a lot of cases when we were using Goliath at PostRank when we just wanted to return a bare string and, as you noted, the .each disappeared between 1.8 and 1.9.

I don't see us changing this, but I'll defer to @nolman and @igrigorik to see what they think.

igrigorik commented 11 years ago

I believe we should be doing an IO conversion or something similar? Since we were never 1.8 compatible anyway. :)

nolman commented 11 years ago

I am not sure I fully understand the issue.

The sinatra app is calling the goliath app (via net/http or equivalent?) and it JSON.parse's (or equivalent) the response.body from the goliath app? Is the sinatra app just returning that JSON parsed response back out to the world? Or is the goliath app being called from within the same ruby process as the Sinatra app? Or is this a response method/class being re-used between the two applications?

We could satisfy the rack spec that the body should respond to #each and it yields a string but I don't feel too strongly about it either way.

kgrz commented 11 years ago

Apologies for the ambiguity, let me clarify.

Goliath Service: A generic service that receives a JSON object and sends out a JSON object.

Input: "{'text': '#This is markdown'}"

Output: "{'html': '<h1>This is markdown</h1>'}"

Sinatra Part: One of the routes makes a request to the Goliath service mentioned above.

When this setup is run with Thin (Goliath part) and Thin (Sinatra part), there was no problem. But when I used Unicorn for the Sinatra part, the app resulted in an error crashing it. The stacktrace from the Unicorn process suggested it was a Rack::Lint error with the response from the Goliath server.

I'll see if I can reproduce this and put up a Gist asap.

nolman commented 11 years ago

I may be incorrect but what I believe what happened is the following:

The lint error you get on unicorn is because it is running each on a ruby hash which doesn't satisfy the Rack API:

rack: https://github.com/rack/rack/blob/master/lib/rack/lint.rb#L358 1.9.3> {"a"=>1}.each {|a| p a.class } # prints Array

The different behavior between thin and unicorn may be because unicorn enables Rack::Lint in RACK_ENV development: https://github.com/defunkt/unicorn/blob/master/lib/unicorn.rb#L65 Where as thin does not enable Rack::Lint.

I believe that changing the response in Goliath to return a JSON array fixed this because you are now iterating on an array vs a hash.

A http request's response.body you get from a webserver (goliath or otherwise) will be a string and would need to be converted to something rack compliant.

kgrz commented 11 years ago

You are right. That's what happened and returning an Array fixed it for the reason that you mentioned. So, does this needs to be added in the Readme/wiki on the repo as a cautionary note? :)

nolman commented 11 years ago

I think that any web server returning a response body would experience this same issue (it's all up to how the response body string is processed in the downstream client). I think it is more of a unicorn defaults or rack spec responsibility than goliath.

That said it may be useful to formalize the expected def response(env) return value. I don't believe that is currently done anywhere (I could be wrong) however I don't feel too strongly for or against it.

Sorry for the late reply.

edit: forgot to cc @dj2 and @igrigorik to see what they think