unicorn Ruby/Rack server user+dev discussion/patches/pulls/bugs/help
 help / Atom feed
* Rack::Request#params EOFError
@ 2017-03-21 18:44 John Smart
  2017-03-21 19:06 ` Eric Wong
  0 siblings, 1 reply; 3+ messages in thread
From: John Smart @ 2017-03-21 18:44 UTC (permalink / raw)
  To: unicorn-public

I found an interesting bug today.  When sending a form multipart >
112KB, I noticed that Rack::Request#params was empty. This only occurs
when using Unicorn, and does not occur when using Thin.

I dug into Rack source and noticed that any EOFError raised while
reading the body input stream will cause the post body input stream to
be ignored:

https://github.com/rack/rack/blob/cabaa58fe6ac355623746e287475af88c9395d66/lib/rack/request.rb#L357

When multipart > 112KB, I noticed that Unicorn tees the input stream
to a temporary file.  I was wondering: might Unicorn::TeeInput raise
an EOFError as part of normal operation when reaching the end of the
input stream?  If so, this would break the Rack spec.  I only tested
this on Darwin, still working on a self-contained repro.

I ended up raising client_body_buffer_size to 1MB as a work-around.
I'm wondering if this is a bug?

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: Rack::Request#params EOFError
  2017-03-21 18:44 Rack::Request#params EOFError John Smart
@ 2017-03-21 19:06 ` Eric Wong
  2017-03-26 20:52   ` Eric Wong
  0 siblings, 1 reply; 3+ messages in thread
From: Eric Wong @ 2017-03-21 19:06 UTC (permalink / raw)
  To: John Smart; +Cc: unicorn-public

John Smart <smartj@gmail.com> wrote:
> I found an interesting bug today.  When sending a form multipart >
> 112KB, I noticed that Rack::Request#params was empty. This only occurs
> when using Unicorn, and does not occur when using Thin.
> 
> I dug into Rack source and noticed that any EOFError raised while
> reading the body input stream will cause the post body input stream to
> be ignored:
> 
> https://github.com/rack/rack/blob/cabaa58fe6ac355623746e287475af88c9395d66/lib/rack/request.rb#L357

EOFError should not happen with normal operations supported by
env['rack.input'] (read, gets, each, rewind).

So I'm not sure why rack does not let EOFError propagate up the
stack...

The equivalent Ruby methods (IO#{read,gets,each,rewind})
do not raise EOFError; read and gets should return nil on EOF...

> When multipart > 112KB, I noticed that Unicorn tees the input stream
> to a temporary file.  I was wondering: might Unicorn::TeeInput raise
> an EOFError as part of normal operation when reaching the end of the
> input stream?  If so, this would break the Rack spec.  I only tested
> this on Darwin, still working on a self-contained repro.

No, it should not raise EOFError unless a client sent less than
the Content-Length it declared in the header, or if it sent a
short chunk with "Transfer-Encoding: chunked".

EOFError should be raised to break out of the application
processing entirely if and only if the client decides to disconnect
prematurely.  This is needed to allow unicorn to move onto other
clients.

What unicorn could (and maybe should) do is raise a different
error which is not a subclass of EOFError; to prevent the error
from being caught by Rack (or any other middlewares).

What client are you using?

Is it sending "Transfer-Encoding: chunked" or a Content-Length?

Is nginx in front of unicorn?  If not, does it happen when nginx
is in front of unicorn?

> I ended up raising client_body_buffer_size to 1MB as a work-around.
> I'm wondering if this is a bug?

Not sure, yet.  unicorn isn't meant to handle unreliable
clients; it's designed to talk to nginx.

Thanks.

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: Rack::Request#params EOFError
  2017-03-21 19:06 ` Eric Wong
@ 2017-03-26 20:52   ` Eric Wong
  0 siblings, 0 replies; 3+ messages in thread
From: Eric Wong @ 2017-03-26 20:52 UTC (permalink / raw)
  To: John Smart; +Cc: unicorn-public

> John Smart <smartj@gmail.com> wrote:
> > When multipart > 112KB, I noticed that Unicorn tees the input stream
> > to a temporary file.  I was wondering: might Unicorn::TeeInput raise
> > an EOFError as part of normal operation when reaching the end of the
> > input stream?  If so, this would break the Rack spec.  I only tested
> > this on Darwin, still working on a self-contained repro.

Ping on this.

Eric Wong <e@80x24.org> wrote:
> No, it should not raise EOFError unless a client sent less than
> the Content-Length it declared in the header, or if it sent a
> short chunk with "Transfer-Encoding: chunked".
> 
> EOFError should be raised to break out of the application
> processing entirely if and only if the client decides to disconnect
> prematurely.  This is needed to allow unicorn to move onto other
> clients.
> 
> What unicorn could (and maybe should) do is raise a different
> error which is not a subclass of EOFError; to prevent the error
> from being caught by Rack (or any other middlewares).

I'm still considering this, but I'm also wondering if it'll
break any existing code which relies on Unicorn::ClientShutdown
being a subclass of EOFError

> What client are you using?
> 
> Is it sending "Transfer-Encoding: chunked" or a Content-Length?
> 
> Is nginx in front of unicorn?  If not, does it happen when nginx
> is in front of unicorn?

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-21 18:44 Rack::Request#params EOFError John Smart
2017-03-21 19:06 ` Eric Wong
2017-03-26 20:52   ` Eric Wong

unicorn Ruby/Rack server user+dev discussion/patches/pulls/bugs/help

Archives are clonable:
	git clone --mirror https://bogomips.org/unicorn-public
	git clone --mirror http://ou63pmih66umazou.onion/unicorn-public

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.lang.ruby.unicorn
	nntp://ou63pmih66umazou.onion/inbox.comp.lang.ruby.unicorn

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox