actix / actix-web

Actix Web is a powerful, pragmatic, and extremely fast web framework for Rust.
https://actix.rs
Apache License 2.0
21.6k stars 1.67k forks source link

Cannot force Content-Length header #1439

Closed jendakol closed 4 years ago

jendakol commented 4 years ago

Expected Behavior

I'm able to provide my value to Content-Length header.

Current Behavior

When I use HttpResponseBuilder (e.g. by HttpResponse::Ok()), my custom-provided content-length header value is ignored/overridden by real length of the response body.
This is very unfortunate when implementing HEAD endpoint where the headers should be equal to GET but with an empty body.

Steps to Reproduce

#[head("/update")]
async fn get_head() -> impl Responder {
    HttpResponse::Ok()
        .content_length(20)
        .content_type("application/octet-stream")
        .finish()
}

I also tried:

HttpResponse::Ok()
  .content_length(20)
  .content_type("application/octet-stream")
  .body(Body::Empty);
let mut response = HttpResponse::Ok()
    .content_length(20)
    .content_type("application/octet-stream")
    .body(Body::Empty); // also with finish()

response
    .headers_mut()
    .insert(header::CONTENT_LENGTH, HeaderValue::from(20));

response

Nothing has helped.

Context

I'm trying to implement HEAD endpoint. According to RFC:

The Content-Length entity-header field indicates ... the size of the entity-body that
would have been sent had the request been a GET.

This is currently impossible.

Your Environment

cholcombe973 commented 4 years ago

I just hit the same problem. I can't manually set the content-length header when implementing a HEAD endpoint. I don't want to redirect head requests to GET because I can implement HEAD in an optimized way that is faster. Is there any workaround for this?

omid commented 4 years ago

You don't need to set the content-length, instead set the body. Actix will not serve it on HEAD.

I know in some specific cases you may need the faster way, but when you don't know the size because the whole process is slow and -maybe- dynamic, how do you know the exact size? I still can see some cases, but rarely it's needed. Can you give me a use case, please?

So this code will work:

#[head("/update")]
async fn get_head() -> impl Responder {
    HttpResponse::Ok()
        .content_type("application/octet-stream")
        .body("12345678901234567890")
}

So in this case, get and head methods can be one method in your code.

jendakol commented 4 years ago

@omid Oh. This solution really didn't come to my mind. It's kind of magical, but I guess it's acceptable.
Thanks.

omid commented 4 years ago

So if everything is fine, you can close this issue :-)

robjtede commented 4 years ago

I feel like this shouldn’t be closed unless this is documented somewhere.

omid commented 4 years ago

How can we document it? Can you lead us?

PS: I'm trying to decrease the number of open issues for Actix.

robjtede commented 4 years ago

I think a note on actix_web::web::head should be okay.

jendakol commented 4 years ago

@omid I waited for @robjtede if he's ok but I totally agree with him - this needs to be documented somewhere (it also goes with your goal to reduce issues).
I'd maybe go for new paragraph in Response docs, explaining that Content-Length is set up automatically and it cannot be forced to some value, however, it's compliant with RFC and sets the value accordingly when responding to HEAD - or better, explain responding to HEAD more thoroughly.

Just one more question now crossed my mind: You've said You don't need to set the content-length. I felt like I cannot set it even when I want to, even though there's content_length method on Response - but that seems to do nothing. Can you please explain it?

Thanks!

omid commented 4 years ago

@jendakol it's a valid point. It seems it's useless :thinking:

jendakol commented 4 years ago

@omid Thank you. If I could choose, please make it useful :smile: I believe some "smartness" is fine (that's why I use actix over plain hyper) but IMHO it should be overridable - let the programmer be the king.

omid commented 4 years ago

OK. I can see several scenarios. Could you please approve them @robjtede ?

  1. The following code should return 0 as Content-Length:

    HttpResponse::Ok()
        .finish()
  2. The following code should return 4 as Content-Length:

    HttpResponse::Ok()
        .body("1234")
  3. The following code should return 50 as Content-Length:

    HttpResponse::Ok()
        .content_length(50)
        .finish()
  4. The following code should return 50 as Content-Length:

    HttpResponse::Ok()
        .content_length(50)
        .body("1234")

The last one is a little bit ambiguous! But it's actually similar to number 3.

I already found the areas, this bug exists in both HTTP 1.1 and 2.

robjtede commented 4 years ago

The tricky thing to consider is that middleware can alter response bodies. Example being the Compress middleware which will, obviously, require a different content length to be sent to have a valid response.

omid commented 4 years ago

So it can happen also for the HEAD requests.

I mean, in the response to HEAD, I can send 100 for example, but later in the real GET it can be lesser because of compression. So we always need to have the complete response!

As far as I can see, we don't need the content_length method, at least as pub!

robjtede commented 4 years ago

As far as I can see, we don't need the content_length method, at least as pub!

Exposing this method does seem weird given it doesn't effectively do anything.

jendakol commented 4 years ago

Guys, those are very good points. However, I'm still not sure if it's not a good idea to give the programmer a right to decide what the header value (because, at the end of the day, it's just a header value, right?) will be.
I could live with the removal of this method, but I'd like to have a right to specify it manually (like every other header). I believe I saw this in Apache HTTP client (Java world) where the content-length value is derived automatically except when the programmer specifies his own value - for whatever reason he did it.

robjtede commented 4 years ago

@jendakol I can see the appeal of absolute control over a handler's response but it seems at odds with the purpose of actix-web being a higher level framework. The automatic content-length handling is very much an intentional feature of actix-web and one that cannot be worked around simply in the presence of middleware.

Scenario: You set the content-length in the handler manually. Then a compress middleware compresses the body. Does it now alter your manual content-length header or not?

If so, that violates your "right" to define the output content-length. If not, you produce an invalid HTTP response. This framework currently chooses to guarantee production of a valid HTTP response even in the absence of programmer's perfect knowledge of actix-web's internal handling.

I think a more reasonable feature request could be a seperate way to define handlers that allow raw access to request and response and don't go through the usual middleware flow. What do you think?

omid commented 4 years ago

This "content_length" method for Response is not used internally. We can safely remove it.

There is also a test here exactly for the HEAD method. I couldn't find any useful commit message for this method.

omid commented 4 years ago

So... according to @fafhrd91's comment, sometimes we need to be able to set content-length. As far as I understood, it's required ONLY when we need no_chunking.

My suggestion is to add the possibility to set Content-Length to the _nochunking function.

Setting Content-Length in the Message struct will be still possible (with the help of headers), but not in the Response.

I'll update my PR in a minute with my suggestion.

jendakol commented 4 years ago

All right. Your arguments are valid I guess... :+1: