Support callable body for explicit streaming support. #1745
Conversation
|
This callable body proposal breaks both of the typical scenarios where Is it necessary to remove As streaming responses can already be handled via |
Can you elaborate? I don't understand why this is true. Is it due to headers? Regardless, I don't see why we can't support
I think making the API nicer is a legit reason for change. I imagine |
I agree with you that even if the change only makes the API nicer without adding any features, we should still consider it. In my opinion, a large change to SPEC should require the API be made much nicer, not just a little nicer. How much nicer the API is made by this change is going to be subjective, of course. I also want to check my understanding that this change is only for a nicer API, and it doesn't enable anything not already possible. Maybe this does enable something not currently possible and I missed it? |
It's semantically identical to partial hijack.
I assert that all existing systems which use full hijack can be implemented better (simpler, safer, more compatible) using streaming responses. We already have some examples of this, e.g. async_cable. Servers can continue to support the Rack 2.x hijack model as well as the Rack 3.x streaming model. The Falcon PoC does this. However, removing |
|
AFAICT full hijack can't be implemented on H2 servers (because there is no real socket to hand out). Mainly I care about:
If we could accomplish all of these goals, then I am happy. Removing hijack makes me worried because I don't want to break apps. If we clearly communicate to web server authors and web server consumers that it's OK for a web server to implement multiple versions of the Rack SPEC, then I think my list is satisfied. Giving web server authors explicit encouragement may even help them to optimize their servers (for example |
Current applications that use an evented approach to full hijack and a single threaded approach to streaming responses will not work if you switch full hijack to streaming responses. For example, applications currently using Unicorn with MessageBus. You may argue that a simpler, safer, more compatible approach would just be switch to another web server, but I don't find that argument compelling. I'm assuming by more compatible you are referring to HTTP 2/3 or TLS support, since any approach that removes
Removing full hijack support breaks apps. As
I'm against encouraging servers to support multiple versions of SPEC, and against removing optional full hijacking from SPEC without replacement. Theoretically, we could add |
Why?
I think this is fair. Though I'm still interested in why we shouldn't encourage web server authors to support multiple SPEC versions. It would be nice if we could iterate on the SPEC, and allowing web servers to implement multiple versions would allow us to do that. Only ever being able to add to the SPEC seems painful. |
@SamSaffron are you able to comment on this proposal w.r.t. this use case?
I think it's more nuanced than this. Full hijack is completely optional, so apps that depend on it are already liable to be broken - i.e. it's not a given that the model works or makes sense from one server to the next, or even any other protocol other than HTTP/1.x. The proposed interface is mandatory and thus can be depended on in a Rack 3 compliant server. I am completely against making it optional, this sends an incredibly confusing message to consumers of the interface. To elaborate on the above point, it's not clear how we could even make full hijack work with HTTP/2, since the way headers are sent is entirely different. It gives a false sense of security because it ties apps to a very specific protocol. The entire point of Rack is to be an abstract wrapper, yet we provide this protocol specific escape hatch which violates all the goals of the project. It's like "we couldn't figure out how to do streaming, so here is the socket so you can figure it out for yourself, good luck with that!". The reason to remove full hijack is because it's fundamentally unworkable as an abstraction going forward, without emulating the entire HTTP/1 behaviour at the wire level for HTTP/2 and HTTP/3 clients. We basically dug our own hole on this, and this PR is a way to try and get out of it. The only "valid" use cases I've seen for full hijack is upgraded HTTP/1 connections, including server sent events (SSE) and WebSockets. Both concerns are handled better with streaming responses managed by the server itself, so application authors would be wise to migrate to the new model. Existing applications can continue working using Rack 2.x and this should be around for a long time if Rack 1.x is anything to go by. Falcon supports Rack 1.x and 2.x, and I imagine that servers will want to support both interfaces. This is not the only breaking change that's been adopted for Rack 3.x, so from a compatibility PoV, servers may want to retain Rack 2 and Rack 3 compatibility shims, or may prefer to have major versions that continue to be supported which implement different SPECs. Completely in agreement with @tenderlove on iterating on the SPEC (e.g. as proposed) and servers being compatible with Rack 2 and Rack 3 makes total sense to me. Assuming this, there is no risk to backwards compatibility except when Rack 2.x becomes a distant memory. |
Removing something from SPEC without replacement and then telling servers to implement an old version of SPEC if they want it sends the wrong message in my opinion. If we have a replacement for full hijacking, then we can replace it in SPEC and encourage web servers to implement the new SPEC. What if a server wants to continue to support full hijacking and also wants to support
I'm not proposing we never remove anything from SPEC. I'm simply proposing we don't remove something optional without a replacement when it has common use cases (even if we don't like those use cases).
By your own admission, by far the most common use of hijacking is for full hijacking and not partial hijacking. If partial hijacking was not required by SPEC, and isn't used much, why should |
Hmm, what if it takes me 30 seconds to tell if I should return a 200 vs 408? How do you inform Rack that later on once you figure stuff out you will have your status code and headers ready? Overall I am very much with @jeremyevans . It is possibly we could re-engineer message bus so it works with the new interface, it does not defer decisions on status code and headers. That said, getting chunking right under all conditions is hard, see: https://github.com/discourse/message_bus/blob/e30ed86c9e29679b7fd090bb797007f3933efbef/lib/message_bus/client.rb#L258-L259 There is huge potential for having disparity in implementation of chunked encoding between servers which can break apps erratically. Killing off full hijack would certainly break this hack, with no option of a replacement: https://github.com/discourse/discourse/blob/42f6c9b6b9db81e2a6a9ca45e76f067980525f67/lib/hijack.rb , that would be a huge shame. Regarding HTTP/2 ... to me this feels like an elusive pipe dream to have this built into the app for many upcoming years, just chuck NGINX in front and your are done. |
Ya, that makes sense. And removing a feature with no replacement doesn't seem to align well with my "don't want to break apps" line item 😆 My biggest problems with full hijack are (and I'm quoting the SPEC here): This seems awful from a user's perspective and a portability perspective (users now have to know how the webserver internals work, and apps written to use this are coupled to the implementation). The other thing is, from what I understand full hijacking is for when people just want the websever to act essentially as a TCP connection manager. Rack is targeted towards the web, so I think this particular feature is a poor fit. Despite my feelings towards the hijack feature, I don't see the harm keeping it (and it's still optional!!) until we figure out either a replacement or a good way to deprecate. @ioquatix is keeping the optional full hijack feature around a show stopper for adding |
|
Perhaps we can change the spec to inform consumers that they will always get a minimal IO instance?
Certainly does not fill with confidence. Maybe:
Also ... returning 418 "I am a teapot" is getting tired, in some ways All for making another options for streaming, but against killing off the old stuff. |
Ya, I think we should probably define this, but lets do it in a different ticket. The inability for middleware to understand that a request was hijacked is definitely a problem. |
Why should this matter? On an event driven server, you can take 30 minutes or 30 days if you want. On a server like Puma, are you assuming that putting it into a background thread mitigates the issue? Because unfortunately, if you don't handle this correctly you will run out of threads.
Not sure where I said this. Completely disagree with this as an argument for why full hijack is a good idea. It's a terrible idea. Partial hijacking is by far the most compatible and useful, because it fits the semantic model of HTTP.
This 100x. If we can get this merged just by retaining hijack, I'm okay with that, but I think it sends a very confusing message. We should definitely continue this discussion with a plan to deprecate and remove it. It doesn't belong in Rack IMHO. |
Sorry, maybe I misunderstood. In the section on
I never said full hijacking was a good idea, and I apologize if I implied that. Even if everyone agrees it is a bad idea/terrible hack, it is commonly used and we should support it until we can replace it. I think the consensus is that partial hijacking is not a replacement for full hijacking. While it is only supposition on my part, I would think if partial hijacking was far more useful, it would be more common than full hijacking. |
|
I have restored hijack to the lint code. I understand why you might want to retain it in terms of compatibility, but from my perspective I don't see that we want to carry this baggage forward. Rack 3 is allowed to and has made make breaking changes. Servers can and will support both for a long time. To me, that's good enough and a fair reason to remove it from Rack 3 which sends a clear message about it's state going forward. That being said, I don't want to hold up this PR and they are actually largely independent from a technical perspective (but not a developer usage perspective). I will still argue for removal and if not that then at least clear deprecation. I don't see that there is any future in "full hijack". |
Mainly people are using puma/unicorn/passenger, all the mainstream appservers out there are heavily constrained on parallel rack requests, for many reasons. Hijack provides an outlet where you can bolt on some evented processing to appservers with limited concurrency.
We need a usable outlet with full parity (not partial). The long term solution can not be "just stop using unicorn / puma and start using falcon" hijack is about providing a common interface for mixed processing - some stuff is within the threadpool the appserver provides, other stuff is in a consumer managed pool. I agree entirely it is hairy, but disagree with junking it and providing a partial solution instead. |
|
I understand the use case, I just don’t think it belongs in rack IMHO. |
This seems to be an effective summary of my general objection to the no-full-hijack issue: you're seeking to remove something that can be correctly implemented on existing and widely-used servers, and offering in exchange an API that can only work with a newer, differently-shaped, server model. Hijacking aside, while I like the idea of simplifying the API that a streaming response needs to implement, I still really dislike the fact that doing so means any middleware must account for two different possible body shapes. If we're going to force this degree of API change, I'd hope we could go further: switch the app It'd also be neat if some middlewares could move off the main app call stack: if they just want to be notified before and/or after the request occurs [and modify headers at that point, say], it sure seems like they could more easily live in an array somewhere. And as a bonus, if we exist in between middleware invocations instead of building them into an opaque linked-list of These are not new ideas, so I'm sure they're been written here before, and this PR is clearly not the place to explore other specific API changes. But anyway: whether something like this is worth the effort, I don't know... but I contend it would work toward improving performance and enriching the API made available to the Rack project's users -- who are middleware, application, and framework developers -- in a way that this also-breaking change does not. I guess I'm just being blinkered, but apart from attempting to force current hijackers to rewrite onto a more server controlled partial-hijack-alike, I'm not seeing how the disruption of introducing callable-body in isolation pays off for our downstreams... I agree it's more intuitive that the explicit enumeration style for the set of developers who need it, but that in my mind that has to weigh against the intuitiveness of the dual interface existing for all middleware authors. (You also twice dismissed explicit enumeration as "less flexible" without elaborating. I recall the discussion in #1600 getting somewhat distracted, but do not recall discussing a way in which Perhaps the core of my feeling is: I don't want to see Rack evolve into a more elegant project, I want Rack to evolve in a way that allows the downstream ecosystem-at-large to be more elegant. I feel that this tends more to the former.* * again, it absolutely does make streaming-body-producing downstreams look nicer, but it does so at the expense of every other potentially-body-aware middleware |
There was a problem hiding this comment.
Implementation wise, I have a few inline comments which should be simple to address.
In terms of the general idea, I do not think the benefits of adding this streaming API outweigh the complexity costs of supporting an additional type of body and breaking any middleware that currently deals with the body.
I'm not actually sure that streaming is easier for an application user with this. We would be expecting users of callable bodies to correctly deal with low-level operations such as 'write', close, flush, closed?, and any exception that any of those methods could raise. Those seem way more like server-level issues than application-level issues. This is similar to the problem with full hijacking, just with a less hacky API.
The minimal application example given is:
run lambda{|env|
[200, [], lambda{|stream| stream.write("Hello World"); stream.close}]
}What happens if stream.close is omitted in this case? What happens if stream.write raises an exception?
While not perfect, I think explicit enumeration via #each already handles the streaming use case well enough, and I am against this proposal.
| REQUIRED_METHODS = [ | ||
| :read, :write, :flush, :close, | ||
| :close_read, :close_write, :closed? | ||
| ] |
There was a problem hiding this comment.
I'm guessing there is a reason, but can you explain why read and close_read would be needed for response streaming, which should be write-only? Is this due to TLS read-before-write issues?
There was a problem hiding this comment.
The provided stream can do both input and output, just like partial hijack.
The problem with explicit enumeration via #each is that:
As you can imagine, with Falcon being on the cutting edge of what's possible, I'm exposed to a lot of use cases which don't fall into the typical "request -> response" model. So, there is definitely a need for something better. Ideally we can standardise that in Rack because that will have a lot of value for people. @matthewd I agree most of your points and introducing a body which has two distinct ways to be used might be considered poor design. Considering partial hijack, we have the similar issue though: def call(env)
return [200, {'rack.hijack' => body}, nil]
endIn this case, should middleware check the headers to see if there is a hijacked body? You can certainly wrap a streaming response but you have to be mindful of how you do it. The conclusions we came to previously is that middleware should never be buffering the response body, and this is why |
|
Thanks for bringing up websockets. I can see how websockets would not be handled by @ioquatix Can you please respond to the following part of my previous response?
|
|
Here is some additional scope/background. Here is the abstraction I ended up with in # Read the next available chunk.
def read
nil
end
# Should the internal mechanism prefer to use {call}?
# @returns [Boolean]
def stream?
false
end
# Write the body to the given stream.
def call(stream)
while chunk = self.read
stream.write(chunk)
end
ensure
stream.close($!)
end
# Enumerate all chunks until finished, then invoke `#close`.
def each
while chunk = self.read
yield chunk
end
ensure
self.close($!)
endThis is a challenging part of the interface and I've gone around in circles on it quite a bit. There are performance and usability constraints layered across different concerns of the system. The interface above is used for both request and response bodies. There are elements of symmetry so that streaming just works everywhere. The most basic interface is This interface is actually sufficient to do full bi-directional streaming. #!/usr/bin/env ruby
require 'async'
require 'async/http'
Async do
endpoint = Async::HTTP::Endpoint.parse("http://localhost:9292")
server = Async::HTTP::Server.for(endpoint) do |request|
input = request.body
output = Async::HTTP::Body::Writable.new
Async do
while chunk = input.read
output.write chunk
end
output.close
end
Protocol::HTTP::Response[200, [], output]
end
client = Async::HTTP::Client.new(endpoint)
Async do
server.run
end
input = Async::HTTP::Body::Writable.new
response = client.get("/", [], input)
output = response.body
Async do |task|
3.times do
input.write("Hello World")
task.sleep(1)
end
input.close
end
output.each do |chunk|
Console.logger.info(response, chunk)
end
endThe problem with this design is that it's inefficient. You end up with at least one internal queue for the "writable body", when you could be writing directly to the stream itself. The approach above is essentially similar to, say Rails streaming, when you return a body which encapsulates a queue, and that queue is enumerated by the server and someone else (thread, task, etc) is responsible for writing to it. When I tried to implement 1 million WebSocket connections like this, the overhead was enough to cause problems, like, it is basically a 2x or more multiplier on your per connection cost. The streaming model proposed here is returning a closure which takes the output stream as an argument, and that's a fundamentally different model and requires that the user code is executed in the context of the server request/response loop. This is more tricky and the server needs to know that this is what's being requested of it. In addition, the code is directly exposed to the underlying stream, rather than a wrapper around the input and output bodies (although that is semantically equivalent, just less efficient). That's the model exposed by something like The problem with The biggest issue we face here is middleware which assumes that the response body is basically an class ETag
def call(env)
_, _, body = @app.call(env)
if Rack::Body[body].streaming?
# Ignore.
else
# generate ETag?
end
end
endWhen I tried to make WebSockets work within a rails controller method, it was very ugly and unintuitive: What we need here is a standard interface for saying: I have a response which needs direct access to the underlying connection. And the server needs to be aware of what to do. And middleware needs to not break. If partial hijack is the answer, why was it not adopted by Rails? Let me also add that WebSockets add their own layer of complexity: Basically, HTTP/1 and HTTP/2 have completely different mechanisms for negotiating a WebSocket stream. The adaptor for Rack basically doesn't have enough abstraction here to deal with the complexity.
If If def initialize(app)
# ...
# Ping all active connections
Async do
@connections.each do |connection|
begin
connection.ping # exception raised
rescue
connection.close
end
end
sleep 1
end
end
def start(stream)
@connections << prepare_websocket(stream)
end
def call(env)
return [200, [websocket headers], self.method(:start)]
endI don't think this is materially different from a partial hijack. If you don't call |
|
@ioquatix can you update your PR to address @jeremyevans's comment here? I read through this thread again, and AFAICT that's the only thing outstanding (someone please correct me if I'm wrong). |
|
@tenderlove I am about to go swimming but after that I'll review/address the comment and update/rebase the PR. |
|
Thank you! |
ioquatix
force-pushed
the
streaming
branch
7 times, most recently
from
5d19f19
to
32f0fe5
Compare
|
Okay, I've added @jeremyevans's suggestion regarding |
|
I've rebased falcon on this proposal and it works as expected. With the proposed language around each/call, it also now works with Rack 1.x and 2.x where we use |
|
I've updated the PR to fix some duplicated text in the spec and updated the commit message to better reflect the changes. |
|
Okay, both PoCs are updated, Puma and Falcon, and both are working. |
Uses "callable body" - rack/rack#1745 - puma/puma#2740 This doesn't work in Puma 5, it will error like this 2022-10-28 08:35:53 +0200 Read: #<NoMethodError: undefined method `each' for #<Proc:0x00000001050b2fd8 /Users/dentarg/local_code/testssl.web/app.rb:44 (lambda)> res_body.each do |part| ^^^^^>
Uses "callable body" - rack/rack#1745 - puma/puma#2740 This doesn't work in Puma 5, it will error like this 2022-10-28 08:35:53 +0200 Read: #<NoMethodError: undefined method `each' for #<Proc:0x00000001050b2fd8 /Users/dentarg/local_code/testssl.web/app.rb:44 (lambda)> res_body.each do |part| ^^^^^>
Allow a rack response body to explicitly opt into streaming by implementing
#callinstead of#each. This interface is desirable for a number of reasons. However, this PR should be considered an experimental spike as there are also a number of downsides, and this essentially represents a breaking change to some typical but unwise use cases.An example would look something like this:
Semantically, it is almost identical to partial hijack. Full hijack is removed. Of course, servers are welcome to support both Rack 2.x hijack, and 3.x whatever.
Current Design
Rack currently supports streaming using
rack.hijack. There are two ways to hijack the underlying socket for a given request: a partial hijack and a full hijack. Not all servers support both modes of operation and neither method is fully compatible with HTTP/2.Partial Hijack
A partial hijack allows the web server to respond with the response status, headers, and then yields the stream back to the application code. It does this by using a special
rack.hijackheader:rack.specific headers with normal HTTP headers which is computationally expensive for the server to detect.Full Hijack
A full hijack allows the web server to completely take control of the underlying socket. This occurs at the point the
rack.hijackis invoked.call(env)method in order for the server continue handling requests.Supporting HTTP/2
It is possible to support partial hijack in HTTP/2. The hijacked stream is multiplexed the same as any other connection. However, such a design cannot be thread safe without significant design trade-offs. Full hijack is semantically incompatible with HTTP/2.
Typical Scenarios
There are several typical scenarios where
rack.hijackhas been used.ActionCable
ActionCable uses full hijack in order to negotiate and communicate using WebSocket protocol. Such a design requires the ActionCable server to run in the same process as the web server.
MessageBus
Actually on Falcon
MessageBus uses full hijack in order to keep the socket alive and feed messages occasionally.
Design Options
There are three possible directions we can go in.
Rack Hijack
We can continue to adapt and bend this model to suit. It currently works, but it will present issues going forward w.r.t. HTTP/2 and generally feels like a hack.
Callable Body
As proposed here, some method on the body other than
#eachis used to determine a streaming response.[200, [], self.dup].Typed Body
As an alternative, using a module or class to indicate that a response body is streamable, e.g.
Rack::Streamable. Streaming would be determined by:BodyProxy?Explicit Enumeration
It's entirely possible to return a body which can yield chunks of data, i.e.
This is already possible with Rack 2.x, however it's not as flexible as a full stream interface. This has been discussed.
rack.input).