[Catalyst-dev] IO bug in Catalyst refactored

[Dan Kubb]

>> I don't have a bug fix to Catalyst, since I rarely do raw IO
>> stuff -- I did try playing around with using IO::Select's
>> can_read within C::E::prepare_body(), but I didn't have any
>> luck getting it to work properly.
>
> Yes this stuff is very tricky, i already experimented with timeouts,
> but those won't wait longer than your 5 seconds by default...
> Btw. Kudos to you for testing the new features!

I dusted off a copy of Network Programming with Perl and found
a solution that works well with the Test and HTTP engines and
solves the IO bug I found.

Basically, I kept the non-blocking reads and updated the read
loop to use select().  The alternative was some sort of "busy loop"
where I'd check sysread() over and over until data was returned..
but I think select() is a better choice.

Due to the nature of non-blocking sysread() calls, you actually
have to check for several different return states, which accounts
for some of the conditionals in the code.  The states that you
need to check for are:

   1. sysread() reads in 1..N bytes.  Returns the number of bytes read.
   2. sysread() attempts to read but no data is available.  Returns  
undef
      and sets $! to EWOULDBLOCK.
   3. sysread() reads, but hits the eof.  Returns numeric 0.
   4. sysread() encounters an error.  Returns undef and sets $! to
      the error code.

Granted #3 could probably be ignored, but if an engine ever uses
a file handle instead of a socket this will likely be important.

Sebastian was probably running into problems with this because the
STDIN and STOUT handles that are created in C::E::Test doesn't
return a true value for fileno(), which is needed by the vec()
call that select() uses.  That means you can't use a select()
call with those handles, and a separate prepare_body() is needed
for C::E::Test's handles.  (well not needed, you *could* put it all
in C::E::prepare_body(), with conditionals for the two branches,
but -- which is what i did originally -- but I think the solution
in the patch is cleaner.)

The patch also simplifies the code in Catalyst::Engine::HTTP
as well as correcting a few subtle bugs:

   - Allows GET and HEAD responses to have the same Content-Length.
   - Doesn't clobber the status if you explicitly set it by
     $c->res->redirect($loc, $status)
   - When setting %ENV from the incoming headers in C::E::HTTP
     if it already existed it was appending "; $name" to $ENV{$name}
     rather than "; $value" as it should be.
   - The "\n" logical newline was being checked for where \012
     should've been used.  ("\n" isn't the same on all platforms)
   - The Set-Cookie header was being sent in ALL cases, even
     if no cookies were actually used.  This could cause problems
     if you wanted the responses to be as "cacheable" as possible.
   - The Date header wasn't sent for HTTP engine responses.

Let me know if you have any problems with anything in the patch.
I have commit access so I could add it myself, but I wanted to
show it to the group first.

--

Thanks,

Dan
__________________________________________________________________

Dan Kubb                  Email: dan.kubb at autopilotmarketing.com
Autopilot Marketing Inc.  Phone: 1 (604) 820-0212
                             Web: http://www.autopilotmarketing.com
__________________________________________________________________



-------------- next part --------------
A non-text attachment was scrubbed...
Name: nonblock.patch
Type: application/octet-stream
Size: 13323 bytes
Desc: not available
Url : http://lists.rawmode.org/pipermail/catalyst-dev/attachments/20050921/1a039204/nonblock-0003.obj