Better support for connection upgrade and bi-directional streaming. #101
Conversation
There was a problem hiding this comment.
I'm not sure This is required for Rack 3 is accurate. I cannot find anything in the Rack 3 SPEC that requires support for the upgrade header or that bodies cannot be buffered. The Rack 3 spec does state that middleware cannot call each, but it says nothing about what servers can do with bodies.
Switching from buffered to unbuffered mode for bodies can be a denial-of-service risk to applications, so I think that should be opt-in. Please remember that the vast majority of webrick use is request/response and not streaming. This is obvious currently because streaming doesn't work at all, but even if streaming did work, the vast majority of webrick use would still be request/response. We can enable streaming support on an opt-in basis without exposing existing webrick users to denial of service vulnerabilities.
| if @bodytempfile | ||
| @bodytempfile.rewind | ||
| IO.copy_stream(@bodytempfile, socket) | ||
| else | ||
| @body.call(socket) | ||
| end | ||
| @sent_size = size | ||
|
|
||
| if content_length = @header['content-length'] |
There was a problem hiding this comment.
Turns out that since @sent_size is set to 0 in initialize, this doesn't currently have an effect. Still, this conditional seems like a better approach.
There was a problem hiding this comment.
Yes I'm fine with that behaviour, or alternatively we could set it to some other value, e.g. nil to represent "unknown".
lib/webrick/httpresponse.rb
Outdated
| elsif @body.respond_to? :call | ||
| make_body_tempfile | ||
| else | ||
| if @body.respond_to?(:bytesize) | ||
| @header['content-length'] = (@body ? @body.bytesize : 0).to_s |
There was a problem hiding this comment.
(@body ? @body.bytesize : 0).to_s looks weird if you already know that @body responds to bytesize. With the change in this PR, probably this should be @body.bytesize.to_s
There was a problem hiding this comment.
Ah yeah, good call, I just copied the previous code.
The streaming body is expected to be streaming, at least to the extent required to support WebSockets, and it should behave like the previously defined full hijack which states (IIRC something to the effect of) that reading and writing should be full-duplex. |
The Full Hijack section does require that a single IO instance is used that The Partial Hijack section (which Streaming Body is equivalent to) does say You are correct that a buffered approach would not work for WebSockets, so in practice, any server that wants to implement WebSockets needs to use an unbuffered approach for them. So even if buffering is not explicitly forbidden by SPEC, it wouldn't be appropriate for WebSocket use. |
|
I think the action item here is, if we can provide additional clarifications in the spec, if you think of anything we can add to make this clearer, please open a PR :) |
Previously, there was no way to implement connection upgrade or proper streaming. The systems I've checked which attempt to do this end up monkey patching webrick. So, let's make it possible without monkey patching. This is required for Rack 3.
This allows streaming which uses
connection: closewhen no content-length is known. It also avoided buffering the entire response which causes problems for streaming real-time responses.