Note: for Rails compatibility, this change will require a corresponding change in Rails's ActionDispatch::Response::RackBody to effectively revert rails/rails#16793 (overriding #to_ary to return nil), which is a hacky work-around for #734, which was caused by the hacky fix (5b251a9) to #419 that has since been fixed more properly in #1453. 😅

I'd consider downgrading to a "should" -- if someone wants to create a CacheAllBodiesEvenBigOnes middleware, to me that should be "consciously deviating from the recommended path" not "breaking the letter of the spec".

I also don't think we should actually call the detected to_ary... that seems avoidable, yet opens up weirdness where the actual response could differ based on whether your parent middleware used to_ary.each or plain each -- which is fixable by defining that those must produce identical results, as we do with to_path... but at that point the body-author can handle that themselves. (I also don't think to_ary should absolve you of the need to check for close.)

I noodled with a few possible phrasings, but couldn't come up with a compelling specific suggestion that conveys "[if you replace the body, and you are consuming the original] you should yield approximately as often [or at least not less often] than the body you're wrapping; if that wrapped body responds to to_ary, then you may ignore this and iterate as eagerly as you like".

I think we might need a BodyProxy subclass that defines to_ary too -- generally, I don't think "you may iterate this body freely if you have need" does or should necessarily imply "please do so at your earliest convenience"... and contrariwise, a middleware that needs after-close handling should not hide the eager-iterability of its enclosed body.

Thanks all for the comments and feedback so far!

@jeremyevans thanks for suggesting more specific wording (and for reminding me that the spec was generated from the lint comments!).

• I really like the line about middleware not calling #each directly, I think it makes the interface very clear.
• I think the line about what exactly can be done with the object returned by to_ary could be left out. The intent of the proposal is to make to_ary an action that fully 'consumes' a Rack body, so the return value from that method is just a standard Ruby Array of Strings and no longer the Rack 'Body' at that point. I don't think anything prescriptive beyond that is needed.
• I moved the spec changes into Lint comments, and added some Lint behavior (plus spec tests) as an implementation of this proposal.

@matthewd thanks for your detailed comments as well.

• I think downgrading 'must' to 'should' muddles the proposal enough to make it not worth adding to the Rack spec in the first place (since it's primarily concerned with formal requirements, not a 'recommended path'). Specifically, it would make the Rack::Lint implementation impossible (or at least much more difficult). I'd be interested in others weighing in on this as well however.
• I think actually calling to_ary is necessary. Beyond the specific case I already mentioned (to prevent confusion caused by narrow workarounds such as #to_ary returning nil), more generally, yielding individual strings via #each isn't always implemented in the same way as producing an array of strings, even if the end result is equal. For example, consider an Array (a common Rack body type) - to_ary is a no-op, while iterating via #each to produce a new array can be an expensive operation for large content. I do like the suggestion that the content produced must be identical to #each to match #to_path, though.
• I think it should be the to_ary implementation's responsibility to call close after iterating through its content. I think it makes sense for a custom content class to encapsulate all of the details of its own coercion (iterating plus closing), without leaking any of that out to the code consuming the returned array. It makes things much easier to reason about.
• Rather than needing more BodyProxy subclasses with this proposal, I believe that with close better encapsulated, BodyProxy actually becomes less necessary for middleware to eagerly-consume content. See the changes to ETag and ContentLength in this PR.

I did another pass on the proposed spec changes while trying to consider all the comments raised. It's quite a bit more verbose, but less ambiguous:

Middleware must not call +each+ directly on the Body.
Instead, middleware can return a new Body that calls +each+ on the
original Body, yielding at least once per iteration.

If the Body responds to +to_ary+, it must return an Array whose
contents are identical to that produced by calling +each+.
Middleware may call +to_ary+ directly on the Body and return a new Body in its place.
In other words, middleware can only process the Body directly if it responds to +to_ary+.

If the Body responds to +close+, it will be called after iteration. If
the original Body is replaced by a new Body, the new Body
must close the original Body after iteration, if it responds to +close+.
If the Body responds to both +to_ary+ and +close+, its
implementation of +to_ary+ must call +close+ after iteration.

Note also that the final paragraph replaces the existing paragraph on +close+. (In effect, this PR clarifies exactly how 'the body is replaced by a middleware after action'.)

This looks great to me, the clarity is excellent. I really like the direction here.

I'm mostly OK with the revised language, except that I think we should change:

If the Body responds to both +to_ary+ and +close+, its
implementation of +to_ary+ must call +close+ after iteration

to:

If the Body responds to both +to_ary+ and +close+, middleware
that call +to_ary+ on the Body do not need to call +close+.

We shouldn't describe how Body objects must work internally, only the interface that middleware should use to interact with them.

I think we should also change:

Middleware may call +to_ary+ directly on the Body and return a new Body in its place.
In other words, middleware can only process the Body directly if it responds to +to_ary+.

to:

Middleware may call +to_ary+ on the Body and use the result as a new Body.
In other words, middleware can only process the Body if it responds to +to_ary+,
and must use the result of +to_ary+ instead of operating on the Body directly.

The reason for this change is to make it clear that you cannot operate on the Body itself just because the body responds to to_ary. You may make sure you are dealing with the result of to_ary. I want to forbid this:

if body.respond_to?(:to_ary) && body.respond_to?(:[])
  body[0] = 'foo'
end

You have to do instead:

if body.respond_to?(:to_ary)
  body = body.to_ary
  body[0] = 'foo'
end

Additionally, you already mentioned that I think actually calling to_ary is necessary, but I think the previous language was too ambiguous for that, and language I am recommending makes it more clear.

Finally, we should probably use a separate heading for this new section such as Middleware interaction with the Body, because I want to be clear that this section is not relevant to servers, who should always use each and close and not to_ary.

@jeremyevans thanks again for your comments and for taking a close look at the proposed language. I think we're getting very close!

Agree with leaving out the line about to_ary calling close internally, since that part isn't strictly a concern of the middleware interface. From the interface perspective, perhaps we can say that the original Body must be closed only if it's "replaced by middleware without calling to_ary".

Also agree with saying that middleware "must use the result of to_ary" - perhaps this could be made even more precise by saying middleware "must not call any further methods on the original Body" after calling to_ary?

It might be hard to forbid code like body[0] = 'foo' as in your example, without a stricter rule like "Middleware must not call any methods on the original Body not listed here" - which might work but sounds a bit drastic (maybe worth thinking about separately).

Finally, we should probably use a separate heading for this new section such as Middleware interaction with the Body, because I want to be clear that this section is not relevant to servers, who should always use each and close and not to_ary.

I actually expect servers to use to_ary when available as well. I mentioned one use-case which is to buffer non-streaming responses to minimize the number of write syscalls on the socket. Many Rack servers already do optimizations like this in practice, typically by checking for an Array body (sometimes a single-element Array) as a special-case. That said, separating the body-object interface from the middleware/server interface in this section could make the added parts a bit clearer, I can try out a draft of that soon.

For now, here's another draft with the changes mentioned:

Middleware must not call +each+ directly on the Body.
Instead, middleware can return a new Body that calls +each+ on the
original Body, yielding at least once per iteration.

If the Body responds to +to_ary+, it must return an Array whose
contents are identical to that produced by calling +each+.
Middleware may call +to_ary+ on the Body and use the result as a new Body,
and must not call any further methods on the original Body.

If the Body responds to +close+, it will be called after iteration. If
the Body is replaced by middleware without calling +to_ary+,
the original Body must be closed first, if it responds to +close+.

Many Rack servers already do optimizations like this in practice

Can confirm.

@wjordan I love the work you are doing here. It is clear to me that you (and everyone else) is drawing on a huge level of knowledge and the result is a clearer, more precise and easier to implement SPEC. Thanks so much and I look forward to when we can merge this!

I don't see this in the changelog. In which version is this included?

I don't see this in the changelog. In which version is this included?

@swrobel This wasn't released yet and you can find it only in main branch. It wasn't backported to current "production" branch of 2-2-stable.

@swrobel This wasn't released yet and you can find it only in main branch. It wasn't backported to current "production" branch of 2-2-stable.

Thanks for clarifying @simi! I do see there's a changelog section for 3.0 unreleased items, so it seems it would be helpful to have there.

@swrobel This is already present in CHANGELOG: Middleware must no longer call `#each` on the body, but they can call `#to_ary` on the body if it responds to `#to_ary`.