Does Sinatra violate the Rack protocol? Breaks when used as middleware for Rails 3.1. #325

Closed
jnicklas opened this Issue Jul 11, 2011 · 9 comments

Comments

Projects
None yet
2 participants
@jnicklas

Seems like Sinatra is trying to set a Content-Length header for any request routed through it, this seems to work fine as long as Sinatra is used on its own, but when used as a middleware this seems to break. The problem is that Sinatra calls #to_ary on the body, which for example Rails 3.1 implements to return the full Rack response, and not the body. The offending code is here.

The Rack protocol says this about the body:

The Body
The Body must respond to each and must only yield String values. The Body itself should not be an instance of String, as this will break in Ruby 1.9. If the Body responds to close, it will be called after iteration. If the Body responds to to_path, it must return a String identifying the location of a file whose contents are identical to that produced by calling each; this may be used by the server as an alternative, possibly more efficient way to transport the response. The Body commonly is an Array of Strings, the application instance itself, or a File-like object.

It only enforces that the object must repond to #each, there is no mention of #to_ary anywhere. Effectively Sinatra seems to be violating the Rack protocol by calling a method on the body which is not part of the protocol, which can and does lead to unexpected results.

It's questionable whether Sinatra should be calling #each on the body either, since that may have undesired side effects, such as the pattern in Rails 3.1 where a proxy object is returned, which is supposed to delegate a task until later, such as closing the DB connection. Sinatra could prematurely close this connection. It seems to me like Sinatra should not touch any Rack response it has not created itself in this way.

@rkh

This comment has been minimized.

Show comment Hide comment
@rkh

rkh Jul 11, 2011

Owner

How do we identify response we've created our self? To check for to_ary is a common check to detect whether an object is like an array.

Owner

rkh commented Jul 11, 2011

How do we identify response we've created our self? To check for to_ary is a common check to detect whether an object is like an array.

@ghost ghost assigned rkh Jul 11, 2011

@jnicklas

This comment has been minimized.

Show comment Hide comment
@jnicklas

jnicklas Jul 11, 2011

@rkh but what kind of Array? The fact that it is an Array tells you nothing, it could contain anything. Granted, it's probably an array, and it probably contains the body, but apparently not always, and not with Rails 3.1. The Rack protocol doesn't say anything about to_ary, so it's not safe to assume that it returns anything sensible.

@rkh but what kind of Array? The fact that it is an Array tells you nothing, it could contain anything. Granted, it's probably an array, and it probably contains the body, but apparently not always, and not with Rails 3.1. The Rack protocol doesn't say anything about to_ary, so it's not safe to assume that it returns anything sensible.

@rkh

This comment has been minimized.

Show comment Hide comment
@rkh

rkh Jul 11, 2011

Owner

Yes, I agree we should change it, but I'm not yet sure how. We could do Array === body. The Rack spec does not have an option for figuring out whether it's safe to loop through the body before sending it to the client. And that would totally kill the new streaming feature of Rails 3.1.

Owner

rkh commented Jul 11, 2011

Yes, I agree we should change it, but I'm not yet sure how. We could do Array === body. The Rack spec does not have an option for figuring out whether it's safe to loop through the body before sending it to the client. And that would totally kill the new streaming feature of Rails 3.1.

@jnicklas

This comment has been minimized.

Show comment Hide comment
@jnicklas

jnicklas Jul 11, 2011

I've created a pull request for a partial fix: #326

This at least fixes the issue with strange methods being called on #body. Some work should probably be done to make sure that this doesn't break when the streaming is used (we're not using it, so I didn't check). But it might actually already work, since we only iterate with #each when Content-Type and/or Content-Length are not set, I'm assuming that Rails is a good citizen and already sets these. It's not ideal, but at least it's something. What do you think?

I've created a pull request for a partial fix: #326

This at least fixes the issue with strange methods being called on #body. Some work should probably be done to make sure that this doesn't break when the streaming is used (we're not using it, so I didn't check). But it might actually already work, since we only iterate with #each when Content-Type and/or Content-Length are not set, I'm assuming that Rails is a good citizen and already sets these. It's not ideal, but at least it's something. What do you think?

@rkh

This comment has been minimized.

Show comment Hide comment
@rkh

rkh Jul 11, 2011

Owner

Rails cannot set the Content-Length upfront if they stream while generating the content.

Owner

rkh commented Jul 11, 2011

Rails cannot set the Content-Length upfront if they stream while generating the content.

@jnicklas

This comment has been minimized.

Show comment Hide comment
@jnicklas

jnicklas Jul 11, 2011

Ahh, yeah point taken. How do they solve that though, since the Rack protocol requires Content-Length to be set?

Ahh, yeah point taken. How do they solve that though, since the Rack protocol requires Content-Length to be set?

@rkh

This comment has been minimized.

Show comment Hide comment
@rkh

rkh Jul 11, 2011

Owner

It does not require it to be set. Content-Type needs to be set.

Owner

rkh commented Jul 11, 2011

It does not require it to be set. Content-Type needs to be set.

@jnicklas

This comment has been minimized.

Show comment Hide comment
@jnicklas

jnicklas Jul 11, 2011

Ahh, indeed. Why are we setting this at all then?

Ahh, indeed. Why are we setting this at all then?

@rkh

This comment has been minimized.

Show comment Hide comment
@rkh

rkh Jul 11, 2011

Owner

By RFC 2616, there are four ways the HTTP client knows that we're done with sending the body:

  • The status code does imply a missing body (1xx, 204, and 304)
  • It was a HEAD request
  • The Content-Length was given and that value has been reached
  • We close the connection

The first two are special cases, obviously.

There are two major downsides of using the last approach: First, the connection is not reusable. HTTP 1.1 offers persistent (reusable) connections, which could down the network overhead. This is simply not possible if the server has to close the connection. Second, a complete response is indistinguishable from an incomplete one where we lost the connection. This could even lead to incomplete pages filling up caches, so this is not only an issue for the one request losing the connection.

Also, setting the Content-Length header lets the client track progress and encourages range requests (i.e. pick up again after the connection has been lost).

Owner

rkh commented Jul 11, 2011

By RFC 2616, there are four ways the HTTP client knows that we're done with sending the body:

  • The status code does imply a missing body (1xx, 204, and 304)
  • It was a HEAD request
  • The Content-Length was given and that value has been reached
  • We close the connection

The first two are special cases, obviously.

There are two major downsides of using the last approach: First, the connection is not reusable. HTTP 1.1 offers persistent (reusable) connections, which could down the network overhead. This is simply not possible if the server has to close the connection. Second, a complete response is indistinguishable from an incomplete one where we lost the connection. This could even lead to incomplete pages filling up caches, so this is not only an issue for the one request losing the connection.

Also, setting the Content-Length header lets the client track progress and encourages range requests (i.e. pick up again after the connection has been lost).

@rkh rkh closed this in 5558cc2 Jul 12, 2011

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