Twirp::Service fails to rewind the rack.input

[ajvondrak]

The Rack app attempts to route the request by reading in the POST body from Rack::Request#body and decoding it on this line: https://github.com/twitchtv/twirp-ruby/blob/c8520030b3e4eb584042b0a8db9ae606a3b6c6f4/lib/twirp/service.rb#L138

Underneath the hood, this is invoking IO#read on the rack.input stream, which forwards the stream to its end. When downstream middleware attempt to re-read from the input stream, they won't get back any data. For exactly this reason, Rack apps that handle the input stream should generally IO#rewind it once they're done reading.

Specifically, we saw this bug cause exceptions when the Honeycomb Rails middleware attempted to parse the HTTP parameters again. See:

• Honeycomb instrumentation breaks using Twirp #44
• ActionDispatch::Http::Parameters::ParseError with Rack app mounted in Rails honeycombio/beeline-ruby#31
• Skip params when unavailable honeycombio/beeline-ruby#39
• Rework the Rails integration honeycombio/beeline-ruby#73

[marioizquierdo]

Thanks for the report!

I understand this causes req.params to raise an exception, but only if a Twirp service is mounted as a Rack app inside a Rails app.

And this would be avoided if Twirp rewinds the request body after reading it:

body_str = rack_request.body.read
rack_request.body.rewind # so downstream middleware can read again
input = Encoding.decode(body_str, env[:input_class], content_type)

Do you know about any performance implications? Is this a common use case for Rack apps after reading a POST request body?

[ajvondrak]

Do you know about any performance implications?

AFAIK, it just involves some pointer manipulation to seek the stream's position to the beginning. This looks to be the case in the source code:

• https://ruby-doc.org/core-2.7.0/IO.html#method-i-rewind
• https://ruby-doc.org/stdlib-2.7.0/libdoc/stringio/rdoc/StringIO.html#method-i-rewind

I imagine it's O(1).

Is this a common use case for Rack apps after reading a POST request body?

Very common:

• https://github.com/rack/rack/blob/766761bb2f92c9053910544629c796d724ed8bf2/lib/rack/request.rb#L456
• https://github.com/rails/rails/blob/e67fdc5aeb2e9583ff1aa5078a5845e283a75ad3/actionpack/lib/action_dispatch/http/request.rb#L325
• https://github.com/phusion/unicorn/blob/bafc04fe666bc5cb8fb7d0534568041964e1e0c2/lib/unicorn/preread_input.rb#L27
• https://github.com/ruby-grape/grape/blob/cf57d250c3d77a9a488d9f56918d62fd4ac745ff/lib/grape/middleware/formatter.rb#L91
• https://github.com/postrank-labs/goliath/blob/bdbf254eca6ff59e5a83d9969db7c5462e06ed61/lib/goliath/rack/params.rb#L26
• Lots of places in https://github.com/rack/rack/blob/master/lib/rack/multipart/parser.rb
• https://github.com/lucasfais/rack-multipart_related/blob/3c068858597f749cbc3288be01ad3f46d249f76a/lib/rack/multipart_related.rb#L27
• Most stream handling will tend to involve rewinding so that downstream users of the stream can re-read it. Take this piece of code in Puma, for example: https://github.com/puma/puma/blob/4f8a85f42147be7ce6f6270052254d9b083c788d/lib/puma/server.rb#L859-L911

It's just generally regarded as necessary housekeeping (e.g., StackOverflow to the effect). Some pieces of code even preemptively rewind the stream before reading it. But not everyone does, so it's safest to rewind to "clean up after yourself". However, there are proposals to remove the #rewind requirement in future versions of Rack altogether: rack/rack#1148

[marioizquierdo]

Sorry it took so long to get back to this issue 😅
@ajvondrak do you mind taking a look at the PR #65 ?

[ajvondrak]

Sorry it took so long to get back to this issue 😅

Believe me, I know how it goes. 😂