-
Notifications
You must be signed in to change notification settings - Fork 217
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
The response part in all the responses should respond to "each" as per Rack spec #239
Comments
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 |
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. |
I believe we should be doing an IO conversion or something similar? Since we were never 1.8 compatible anyway. :) |
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. |
Apologies for the ambiguity, let me clarify. Goliath Service: A generic service that receives a JSON object and sends out a JSON object.
Input: Output: 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. |
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 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. |
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? :) |
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 |
In almost all the examples, the
response
method is something like:As per Rack spec, the third element in the response array should respond to
each
which was true for aString
object in Ruby 1.8. Since 1.9 though, this is not the case. So, should the examples be updated accordingly?The text was updated successfully, but these errors were encountered: