Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Specify rack.provisional_response to send 1xx responses #1701

Closed
wants to merge 1 commit into from

Conversation

casperisfine
Copy link
Contributor

As discussed in #1692

Context

Three years ago the rack.early_hints callback was added to puma. Since then Falcon and Unicorn followed suit, and frameworks such as Rails and Hanami started using it, so I wanted to make it part of the spec in #1692.

However after discussing it, I believe that interface is a bit too narrow, and we might as well allow to return any 1xx status code, with arbitrary headers.

This spec.

The naming is based on the HTTP/1.1 RFC, the 1xx section is named "Informational", but then says:

This class of status code indicates a provisional response

I think rack.provisional_response is the better name in Rack context, but I'd be happy to rename to rack.informational_response or something else.

The callback signature is call(status, headers), it should only be called with 1xx statuses.

The callback doesn't accept a body as the spec is pretty clear that it's not valid:

consisting only of the Status-Line and optional headers

Early Hints

Server which wish to sill expose rack.early_hints can easily redefine it as ->(headers) { env['rack.provisional_response'].call(103, headers) }.

Interested parties and Refs:

Implementation details

I didn't delegate to check_headers because there are two assertions that I don't think make sense in this context:

        ## Special headers starting "rack." are for communicating with the
        ## server, and must not be sent back to the client.
        next if key =~ /^rack\..+$/

        ## The header must not contain a +Status+ key.
        assert("header must not contain Status") { key.downcase != "status" }

cc @jeremyevans

@ioquatix
Copy link
Member

If you don't think the existing check headers makes sense then maybe split it?

@ioquatix ioquatix self-assigned this Jul 30, 2020
@ioquatix ioquatix added the SPEC label Jul 30, 2020
@ioquatix ioquatix added this to the v3.0.0 milestone Jul 30, 2020
Copy link
Contributor

@jeremyevans jeremyevans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are better off just supporting rack.early_hints, until such point as a different provisional response with headers makes sense. I think this will only be used by early hints and nothing else, and trying to add flexibility just for a possible theoretical future use that may never come to pass smells bad. YAGNI. We have at least 3 servers and 3 web frameworks supporting rack.early_hints, and having Rack standardize a different approach will just create churn for all of them. We can always add this more flexible approach later if there is another provisional response that is standardized that would be useful.

In terms of implementation if we do go with this approach, I think we should specify that the headers (2nd argument) provided to rack.provisional_response should have the same format as the headers (2nd element) of a rack response. This reduces duplication and will make it so if we require the rack response headers to be a hash, then that will apply to this case as well. I can't think of a reason we would want the provisional response headers to have a different format than normal response headers.

@casperisfine
Copy link
Contributor Author

and trying to add flexibility just for a possible theoretical future use that may never come to pass smells bad. YAGNI

I understand the sentiment, but this really isn't about YAGNI. It was much more about clean abstractions.

least 3 servers and 3 web frameworks supporting rack.early_hints, and having Rack standardize a different approach will just create churn for all of them.

Yes I agree. It's just unfortunate I guess, but this is the pragmatic way to go.

and trying to add flexibility just for a possible theoretical future use that may never come to pass smells bad. YAGNI

That's kinda what I did, but I could make it more explicit indeed. The problem is that there are things in check_headers that don't really make sense in 1xx response, e.g. rack.* headers.

@jeremyevans
Copy link
Contributor

and trying to add flexibility just for a possible theoretical future use that may never come to pass smells bad. YAGNI

I understand the sentiment, but this really isn't about YAGNI. It was much more about clean abstractions.

least 3 servers and 3 web frameworks supporting rack.early_hints, and having Rack standardize a different approach will just create churn for all of them.

Yes I agree. It's just unfortunate I guess, but this is the pragmatic way to go.

Absent existing support for rack.early_hints in web browsers and frameworks, this approach seems fine. However, considering existing support for rack.early_hints, it seems different/incompatible for no practical benefit. This is definitely not the pragmatic way to go. The pragmatic way to go is rack.early_hints since that is what is already supported. This is trying to guess possible future needs and trying to accommodate them in advance should they materialize. As @tenderlove stated: making it too flexible when there's no reason. We can always add this later if a need for it materializes. Why now?

@tenderlove
Copy link
Member

FWIW I don't have a particularly strong feeling. Since the web servers already support rack.early_hints, that should probably be lifted in to the spec as-is. If popular web servers all do it, then it's really the defacto spec. Rack's job is to define the interface, and if all web servers agree this is a fine interface then I think we're good to go?

Maybe we should pull rack.early_hints in to spec and the postpone discussion of adding the status code until it's more useful? Now that web servers implement rack.early_hints, it's not like they can remove it any time soon because of backwards compatibility. Were we to add a version that accepts a status code, they would have to support two APIs regardless.

I started off saying I don't have strong feelings, but the more I write the more I'm convinced we should lift rack.early_hints in to the spec as-is. I totally understand why we would want to add a status in the future, but that seems like work we can put off until we actually need it.

@ioquatix
Copy link
Member

I need to make it clear again, falcon does not support 103 early hints. It turns early hints into push promises, and the interface defined here is cumbersome for that task.

@jeremyevans
Copy link
Contributor

@ioquatix Just for clarity, would you also prefer rack.early_hints instead of rack.provisional_response because Falcon can't handle generic provisional responses?

@ioquatix
Copy link
Member

ioquatix commented Jul 31, 2020

Falcon can't handle generic provisional responses?

"can't handle" is not quite right. It's more like "is it standardised yet?". What is the value of early hints/provisional response? I don't know any browser supports it, I don't know any HTTP client supports it. Do any load balancers support it? It seems like Fastly supports it? If you have a Rack app running behind Nginx or ELB or some other load balancer... does it work? Is the 1xx response transited correctly?

When I implemented it, I was interested in a way for Rack apps to send push promises via HTTP/2. Because that has been standardised and is supported by browsers. Even if it is probably a useless feature all things considered. The early_hints interface is poorly suited to push promises because it's essentially just a list of headers which you need to parse to get the actual assets that should be pushed.

I have no problem with the proposal here in general - I think we should be considering ways to improve the performance of rack applications.

However, early hints/provisional response:

  • Doesn't feel like a formal standard yet. I checked blogs, RFCs (marked as Experimental), etc. Do we want to incorporate this into Rack only to have it never adopted in practice? What's the bar we want to set?
  • Doesn't feel any better than push promises in practice / no validation of performance improvements in practice. I would really love to know if it's worth investing a huge effort into this. If we can show the latency is improved significantly I'd personally be a lot more excited and push harder to standardise it.
  • Doesn't seem to be supported by clients of any kind AFAIK (e.g. Chrome, Faraday, RestClient, Net::HTTP, etc). It represents a significant departure from the request-response style and that's having an impact on downstream implementations that make (reasonable IMHO) assumptions about request/response flow.

I'm not against this at all, I think the PR is great, but I just think we don't quite know what form this is going to take yet.

Assuming that provisional response is something we want to implement and the above issues are resolved satisfactorily, I think what we should be thinking about is:

  • Are push promises and early hints the same thing or different?
  • Should servers convert from one to the other (i.e. HTTP/2 push promise -> HTTP/1 early hints and vice versa).
  • What interface do we want to expose to apps (bearing in mind we have to support it for the next 10 years or longer).

The answer might be:

  • Let's add env["rack.provisional_response"].call(status, headers). It's not yet fully standardised, but that looks to be the right interface based on the work/discussion done here.
  • Let's add env["rack.push_promise"].call(path, **attributes). It's standardised and the underlying server could map it to early hints or push promises.

Finally, neither of those answers actually solves the problem of "Make sure the user loads these resources as quickly as possible if required." They hoist all the complexity of dealing with "What resources does the user have already and what resources do I want the user to have" into the application. Is that the right approach? I don't know if it should be the responsibility of a rack application to deal with that.

@casperisfine
Copy link
Contributor Author

@jeremyevans

The pragmatic way to go is rack.early_hints

Yes it's what I was saying. rack.early_hints is the pragmatic choice. My sentence must have been confusing.

This is trying to guess possible future needs and trying to accommodate them in advance should they materialize

No. Why I believe this is better than rack.early_hints (on a purely theoretical standpoint), is because it implement a more general concept of the original HTTP1/1 spec, rather than a very specific code and usage recently defined in a new RFC. In my eyes it's just a much cleaner abstraction, which better represent the HTTP protocol.

But I also understand that it's silly to look at the existing support and just throw it away. Even myself I'd feel stupid going back to Eric Wong with a new patch.

@casperisfine
Copy link
Contributor Author

IMHO:

  • Are push promises and early hints the same thing or different?

They are different.

Should servers convert from one to the other (i.e. HTTP/2 push promise -> HTTP/1 early hints and vice versa).

It's a possibility, you do it in Falcon, H2O does it, but it's kind of a middleware / reverse proxy job. I don't think it should be forced that way on users, and I think the Rails implementation is wrong to assume they will be converted to push promises.

For your Falcon use case, what I think would be an elegant way to handle this would be to expose env["rack.push_promise"].call(path, **attributes), and then as an option or middleware, allow env["rack.early_hints"] to be a parser + delegator to rack.push_promise. This way you let your users have control on this.

@ioquatix
Copy link
Member

I don't want to implement rack.early_hints if it's a subset of another more general approach. I don't care that much about existing servers, I'd rather implement the generic interface in falcon than two interfaces where one is more specific than another.

@ioquatix
Copy link
Member

It looks like Chrome will remove support for push promises: https://groups.google.com/a/chromium.org/forum/?authuser=0#!msg/blink-dev/K3rYLvmQUBY/vOWBKZGoAQAJ

Do any mainstream browsers support 1xx responses?

@nateberkopec
Copy link

@ioquatix your link describes that Chromium will support 103 Early Hints if this: https://chromium.googlesource.com/chromium/src/+/master/docs/early-hints.md experiment goes well.

@casperisfine
Copy link
Contributor Author

Chromium will support 103 Early Hints if this experiment goes well.

As we discussed before in this thread or the other, the problem with Chrome's experiment it that it causes a chicken and egg.

I tried to push Early Hints at Shopify, but quickly got asked: what are the gains? The answer being none, added to the risk of breaking the service for older browser made it so that while we didn't totally trash it, I'm having a hard time convincing people to help me put it in production.

Same thing here in this discussion, we circle back between: let's not implement it in X because no browser supports it. And chrome saying they might implement it if some service start using it.

So I really don't know how to get out of this situation.

@ioquatix
Copy link
Member

I implemented a lot of it in falcon, so I'm not in the camp of "let's not implement it in X".

However, my experience is that it's simply not a useful feature.

At best, you save 1RTT with HTTP/2.

With HTTP/1, it's less obvious.

At worst, you waste all the effort of sending the additional responses.

It fundamentally circumvents the request/response interface of HTTP, and so I think many clients (and servers) have a hard time knowing (1) how to proxy requests, (2) what kind of interface to expose and (3) how to do it efficiently.

I think this is just a case where the value is not exceeded by the complexity.

To that end, I'm even considering removing HTTP/2 push from falcon & async-http. It adds so much complexity and yet I don't think I've seen a single use case.

@casperisfine
Copy link
Contributor Author

I implemented a lot of it in falcon, so I'm not in the camp of "let's not implement it in X".

It wasn't meant as an attack on anyone. Just a general annoyance about this deadlocked situation. I just wish all parties would simply implement 1xx provisional responses support simply because it's how the HTTP spec works and not consider wether there's a use case or not.

I just want to be able to be able to craft any valid HTTP response, including 1xx, using the rack API. If implementing 1xx was a lot of work I could understand some reluctance, but from my point of view after implementing it in unicorn, it's really a fairly trivial feature that doesn't cost much.

@ioquatix
Copy link
Member

@casperisfine I understand your position, and I didn't see it as an attack, and I feel your frustration too.

1xx informational responses are not completely trivial.

In a normal interface, you'd have something like:

response = client.request(path, ...)
puts response.status
puts response.read

However, with 1xx responses, this is no longer sufficient.

The only idea I came up with was something like this:

response = client.request(path, ...)

do
  puts response.status
  puts response.headers
  puts response.read
while response = response.next

There are obviously ways you can control this (flags, etc). However, it does impact the client significantly. Bearing in mind this needs to work transparently in proxies, etc. In async-http with HTTP/2 push promises it looks like this:

Async do
  response = client.request(path, ...)
  
  Async do
    while extra_response = response.promises.pop
      # deal with extra response
    end
  end
end

The response containing multiple responses is fundamentally a different semantic model.

Do you have any ideas about how we can make that part work? i.e. from the client side?

@casperisfine
Copy link
Contributor Author

Do you have any ideas about how we can make that part work? i.e. from the client side?

IMHO, the level 0 of support client side would be to simply disregard the 1xx response, and continue reading (possibly with a reset timeout). That's what 100 Continue was intended for:

The 100 (Continue) status code indicates that the initial part of a
request has been received and has not yet been rejected by the
server. The server intends to send a final response after the
request has been fully received and acted upon.

When the request contains an Expect header field that includes a
100-continue expectation, the 100 response indicates that the server
wishes to receive the request payload body, as described in
Section 5.1.1. The client ought to continue sending the request and
discard the 100 response.

If the request did not contain an Expect header field containing the
100-continue expectation, the client can simply discard this interim
response.

e.g. you know the response will take a lot of time to be ready to send, so you send a 100 Continue to tell the client to continue listening.

Then for extended support, meaning allowing client code to act upon the 1xx, I believe it should be handled through a callback. I'm very unfamiliar with Async, but as pseudo code:

client.on_informational_response(103) do |response|
  preload(response.headers['Link'])
end
response = client.request(path, ...)

If no handled is assigned for a code, then it can simply be ignored:

A client MUST be able to parse one or more 1xx responses received
prior to a final response, even if the client does not expect one. A
user agent MAY ignore unexpected 1xx responses.

@ioquatix
Copy link
Member

Push promises have been basically abandoned, so I'm planning to remove this functionality. However, I'm open to exploring what an interface for provisional responses looks like, I'm just not sure how to adopt it or what advantage it holds over a streaming response with link headers sent before the body is generated. Do any browsers support provisional responses with link headers?

@casperisfine
Copy link
Contributor Author

I'm just not sure [...] what advantage it holds over a streaming response with link headers sent before the body is generated.

A streaming response mean you have to commit to a response code (e.g. 200), if later on there's a problem and you want to send a 500, well you can't anymore. That's why streaming response are not a good solution in the vast majority of cases.

Do any browsers support provisional responses with link headers?

Chrome 95 yes: https://chromium.googlesource.com/chromium/src.git/+/refs/heads/main/docs/early-hints.md

@ioquatix
Copy link
Member

HTTP/2 solves the inflight request failure case with stream error semantics.

@casperisfine
Copy link
Contributor Author

stream error semantics

You mean https://httpwg.org/specs/rfc7540.html#StreamErrorHandler ? I don't see how that solves the problem.

Besides, assuming HTTP/2 from Rack seems wrong to me. Even with you want to serve your app with HTTP/2 it's very popular to terminate it with SSL at the load balancer or reverse proxy layer, and then go HTTP/1.1.

@jeremyevans
Copy link
Contributor

But I also understand that it's silly to look at the existing support and just throw it away. Even myself I'd feel stupid going back to Eric Wong with a new patch.

@casperisfine Considering this and that we plan to merge support for rack.early_hints, are you OK with closing this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants