Pipelining Fix/Refactoring #73
Conversation
| @@ -40,21 +42,32 @@ def detach | |||
| end | |||
|
|
|||
| # Reset the current request state | |||
| def reset_request(state = :header) | |||
| def reset_request(state = :standard) | |||
There was a problem hiding this comment.
Wasn't sure what to call it, it's not in charge of knowing whether it's in "headers" any more, just knowing that the connection still has incoming requests and isn't a websocket. Maybe :ready? The only values it can have are :websocket, :closed, or ____. What do we call the blank?
| @parser.readpartial(size) | ||
| end | ||
|
|
||
| def outstanding_request |
There was a problem hiding this comment.
Some minor bikeshedding, but perhaps this should be #current_request
There was a problem hiding this comment.
Good point. Added this a few days after the first one. I'll make them line up.
|
Thanks for the feedback - everything you mentioned should be fixed now. |
|
Looks good now, thanks! 👍 |
|
|
||
| if block_given? | ||
| # Callback from the http_parser will be calling add_body directly | ||
| @on_body = Proc.new(&block) |
There was a problem hiding this comment.
This doesn't need wrapping in another proc does it? @on_body = block should suffice. Given that you're switching on if block_given? you could drop the &block parameter declaration, and make this line @on_body = Proc.new (which inherently consumes the block argument).
|
Looking over this again, I'm really not a fan of the callback-driven body API. I think that Reel should support a Body object that supports a I already landed this and will be keeping it around, but I'll probably try to make that change today. |
|
Okay, I see that API was my fault ;) |
| end | ||
| end | ||
| it 'streams body properly with #body &block' do | ||
| with_socket_pair(8) do |client, connection| |
There was a problem hiding this comment.
Why "8"? If you're going to bother testing a buffer size, use 1, or preferably 1 and >1
There was a problem hiding this comment.
There's no good reason, I was just playing with different numbers to make sure they all worked as expected and I left off on 8. It definitely makes sense to have at least one case that's a byte at a time.
There was a problem hiding this comment.
Yeah after looking at how the tests are written this is probably ok. I'd expect the edge cases to crop up at 1 though
Did some refactoring and made it so the request parser can handle pipelining. This closes #69 and #55 and makes #56 obsolete.
All of the specs pass, and I've run most of the examples and they all seem to work as well as they do on master (Rackup handler examples seem to be broken on master?)
Cleanup Changes:
Reel::Connectiondelegates all of its work toReel::Request::Parser(for socket reads) and Reel::Response::Writer (for socket writes).Reel::Connection::StateErrorif not. No other objects know the parser/writer exist.Reel::RequestandReel::Websocketno longer require a direct link to the request parser - they are initialized with a Reel::RequestInfo object that includes the parsed request headers, url, method, http_version.Reel::Response::Writerknows how to render proper HTTP responses, but will send the socket toResponse#renderif that method exists (for theStreamResponsesubclass) (there was a TODO about extracting this logic).API Changes:
#readcan now only be called on theReel::Request, not connection, since http_parser may accidentally consume bytes of multiple bodies, eachReel::Requestkeeps an internal buffer of bytes parsed but not yet pulled with #read. This buffer is read from before calling@connection#readpartial#<<and#writeto Reel::Request so you can call both#readand#writeon the request itself.Reel::Connection.Parser Details:
The
Reel::Request::Parsermaintains an internal buffer of incoming requests still reading (if body hasn't been read in yet), one currently awaiting response, and a queue of requests to be responded to after the current request is handled. The underlying http parser calls the callbacks on this class when bytes buffered to it add to headers or body.Reel::Request::Parserhas#current_request, which will return the current request being read/responded to, or block and read from the raw socket until a full set of headers has been discovered and the body is ready to read.