Closed ajvondrak closed 3 years ago
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?
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:
I imagine it's O(1).
Is this a common use case for Rack apps after reading a POST request body?
Very common:
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: https://github.com/rack/rack/issues/1148
Sorry it took so long to get back to this issue 😅 @ajvondrak do you mind taking a look at the PR #65 ?
Sorry it took so long to get back to this issue 😅
Believe me, I know how it goes. :joy:
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#L138Underneath the hood, this is invoking
IO#read
on therack.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 generallyIO#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:
44