Skip to content
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

rack.input does not rewind #4

Closed
dekellum opened this issue Feb 13, 2012 · 7 comments
Closed

rack.input does not rewind #4

dekellum opened this issue Feb 13, 2012 · 7 comments

Comments

@dekellum
Copy link
Owner

This issue was first raised in matadon/mizuno#15 and 5b22977 suggests a fix. However my review and testing finds that RewindableInputStream.to_io (from jruby-rack, kirk, et al) also will not yield a functioning rewind. The kirk original also includes ReweindableInputStream to IO adapter (see its input_stream.rb.) So either that needs to be co-opted here, or a better performing JRuby java extension for InputStream to IO implemented.

Also I would prefer to not use temp files for input caching except where absolutely necessary for giant uploads. An in memory implementation with configurable rewind limit might resolve the issue for 99% of use cases without the performance and complexity impact.

JRUBY-5891 is related. However JRUBY is not going to fix this: InputStream.to_io is not going to support rewind without major changes and assumptions.

@iconara
Copy link

iconara commented Feb 5, 2015

This became a real issue in JRuby 9K. I'm using Fishwife with Grape, and it rewinds the request body for some reason, and that raises an Errno::EPIPE. I'm not really sure why this doesn't happen in JRuby 1.7, but the IO has been thoroughly rewritten in 9K so everything works differently.

@iconara
Copy link

iconara commented Feb 5, 2015

I found the same problem in the last Travis run of Fishwife, so you probably already know what I'm talking about: https://travis-ci.org/dekellum/fishwife/jobs/48211628#L446

@dekellum
Copy link
Owner Author

dekellum commented Feb 5, 2015

Thanks for pointing this out @iconara, as I hadn't investigated the specifics of the jruby 9.x travis failure. I'd like to see a clean implementation of input rewind in Fishwife. Happen to know if there is a suitable open source implementation existing that could be leveraged? I'm not sure when I will get the time to write one from scratch.

@iconara
Copy link

iconara commented Feb 6, 2015

I can't say that I do, but I will investigate too. We've tried just about all (J)Ruby Rack servers now and Fishwife is the only one that we haven't yet found any horrible faults with. We're not looking to move to 9K anytime soon, I think a stable version is months into the future, but once we do we will solve this problem one way or the other.

I looked at the suggested fix in Mizuno, but I think it's too complex. I don't have the full grasp of the problem yet so I probably shouldn't make too many judgements, though. From my perspective an upload is either small or big. Small as in it's no problem to gobble it into an in-memory buffer, and big as in we need to buffer it on disk, but that may be too simplistic.

I'm also considering throwing out all Rack frameworks that try to rewind the request body, since it means they read it, and that feels like a pretty stupid thing for a framework to do. It's not hard to OOM an application that behaves that way.

@dekellum
Copy link
Owner Author

Regarding the JRuby 9.x travis failure and @iconara 's question on how this "worked" on JRuby 1.7.x—earlier JRubies InputStream#to_io return an IO objects that responds to rewind. Unfortunately this rewind is a No-Op. JRuby is masking the lack of rewind support and keeping Rack::Lint (used in the tests) happy where it shouldn't be.

In JRuby 9.x, apparently the object no long responds to rewind and instead raises Errno::ESPIPE, which is correct.

dekellum added a commit that referenced this issue Feb 10, 2015
dekellum added a commit that referenced this issue Feb 10, 2015
dekellum added a commit that referenced this issue Feb 12, 2015
@dekellum
Copy link
Owner Author

Rewind support for request bodies is now implemented in the fishwife 1.7.0 release. See the request_body_* configuration options of Fishwife::HttpServer.new.

@iconara
Copy link

iconara commented Feb 13, 2015

That's great, and :request_body_max was a nice extra addition that will make it possible for me to throw away some code. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants