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

handles HEAD requests automatically when there exists a GET route that would otherwise match. 405 Method Not Allowed returns for now. #84

Closed
songww opened this issue Aug 2, 2021 · 15 comments · Fixed by #129
Labels
C-enhancement Category: A PR with an enhancement
Milestone

Comments

@songww
Copy link
Contributor

songww commented Aug 2, 2021

Feature Request

Motivation

Proposal

handles HEAD requests automatically when there exists a GET route that would otherwise match. It does this by stripping the body from the response, if there is one. You can also specialize the handling of a HEAD request by declaring a route for it; won't interfere with HEAD requests your application explicitly handles.

Alternatives

Just like rocket did.

head-requests

@davidpdrsn davidpdrsn added the C-enhancement Category: A PR with an enhancement label Aug 2, 2021
@davidpdrsn
Copy link
Member

I've looked into this and I don't think a clean solution is possible given the current design of the router.

When you compose two routes with head(handler).get(other_handler) neither head nor get knows if they're the "last" route. That is to say a get route has no way to know if there is another route that accepts head that is also defined and so it cannot change its behavior based on that.

One possible solution is to make get accept both GET and HEAD request but always remove the response body on HEAD. This is a breaking change however since head(handler).get(other_handler) would now call other_handler instead of handler on HEAD requests (routes are matched bottom to top).

Not sure if entirely removing explicit matching on HEAD requests would make things less confusing. That way get could always accept GET and HEAD without a head method getting in the way. I think that is quite drastic though, and removing the ability to match on HEAD feels odd. You can also get around it using any I suppose.

I checked with warp (the framework axum is most alike from a design perspective) and it handles HEAD requests the same way as axum does today. So warp::get().map(|| "Hi there!"); does not accept HEAD requests. It seems an issue was opened about that recently so lets see what happens there.

Honestly I would be fine with leaving things as is. User's who want to support HEAD requests can explicitly add handlers for them. In my experience their usage is fairly niche. Don't think I've ever personally sent a HEAD request 🤔 At least not knowingly.

What do you think @songww @jplatte?

@jplatte
Copy link
Member

jplatte commented Aug 2, 2021

Don't think I've ever personally sent a HEAD request thinking

REST services used from browsers commonly need HEAD implemented for all endpoints AFAIK (though there's also many that don't, and I really should look into how that works). But it's really wasteful to hit the DB to generate a response body when the response is then stripped and only the headers returned, so the right thing would probably be to implement head manually, or have a macro that generates both head and get impl from one function, i.e. sth. like

use axum::{
    extract::{Query, UrlParams},
    response,
};

#[get_head]
async fn list_foos(
    UrlParams(user_id): UrlParams<Uuid>,
    params: Query<PaginationParams>,
    db: extension<sqlx::PgPool>,
) -> crate::Result<response::Json<Vec<Foo>>> {
    Ok(Foo::list_by_user(user_id, &pagination, db.deref()).await?)
}

// generates
async fn list_foos(
    UrlParams(user_id): UrlParams<Uuid>,
    params: Query<PaginationParams>,
    db: Extension<sqlx::PgPool>,
) -> crate::Result<response::Json<Vec<Foo>>> {
    Ok(Foo::list_by_user(user_id, &pagination, db.deref()).await?)
}

async fn head_list_foos(
    _: UrlParams<Uuid>,
    _: Query<PaginationParams>,
    _: Extension<sqlx::PgPool>,
) -> StatusCode {
    StatusCode::OK
}

Two notes:

  • This works, but is still not entirely optimal (request extensions are pointlessly cloned)
  • This assumes that the CORS headers without which HEAD requests are pointless are added by a middleware
    • That seems pretty much like the one right way to do things to me, but it still irks me that any headers set within the async fn body would not apply to the HEAD handler

@davidpdrsn
Copy link
Member

This assumes that the CORS headers without which HEAD requests are pointless are added by a middleware

That's a fair assumption to make I think but I see your point.

Mdn mentions that headers should be identical across GET and HEAD so the actual handler (and possibly database) would probably have to be called in case you're setting content-length. Which would then have the same amount of load in the server.

Kinda feels like something that is hard for a framework to get right, in the general case 🤔 Is requiring users to explicitly handle HEAD in the way that makes sense for their app perhaps the best approach or is there a "goo enough" middle ground?

@jplatte
Copy link
Member

jplatte commented Aug 2, 2021

Scrap everything I wrote. It doesn't belong on this issue; I mixed up HEAD and OPTIONS 😅

I have some additional thoughts for supporting HEAD and OPTIONS easier for some response types, but it's super late here, will write that up tomorrow.

@songww
Copy link
Contributor Author

songww commented Aug 3, 2021

We usually use curl -I to test whether it is working properly. It is very effective for scenes where the body is large or does not care about the body.

If a handler is returning static files by tower_http::services::ServeFile, then manually implementing HEAD will be much more difficult than GET.

@davidpdrsn
Copy link
Member

If a handler is returning static files by tower_http::services::ServeFile, then manually implementing HEAD will be much more difficult than GET.

It can be done by making a tower::Service middleware that drops the response body like so:

use axum::{prelude::*, service};
use bytes::Bytes;
use http::Response;
use http_body::Empty;
use std::{
    net::SocketAddr,
    task::{Context, Poll},
};
use tower::Service;
use tower_http::services::ServeFile;

#[tokio::main]
async fn main() {
    tracing_subscriber::fmt::init();

    let serve_file_svc = ServeFile::new("Cargo.toml");
    let app = route(
        "/Cargo.toml",
        service::get(serve_file_svc.clone()).head(DropResponseBody(serve_file_svc)),
    );

    let addr = SocketAddr::from(([127, 0, 0, 1], 3000));
    tracing::debug!("listening on {}", addr);
    hyper::Server::bind(&addr)
        .serve(app.into_make_service())
        .await
        .unwrap();
}

#[derive(Clone)]
struct DropResponseBody<S>(S);

impl<R, S, ResBody> Service<R> for DropResponseBody<S>
where
    S: Service<R, Response = Response<ResBody>> + Clone,
    S::Future: Send + 'static,
{
    type Response = Response<Empty<Bytes>>;
    type Error = S::Error;
    type Future = futures::future::BoxFuture<'static, Result<Self::Response, Self::Error>>;

    fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> {
        self.0.poll_ready(cx)
    }

    fn call(&mut self, req: R) -> Self::Future {
        let future = self.0.call(req);

        Box::pin(async move {
            let response = future.await?;
            // discard the response body
            let response = response.map(|_body| Empty::new());
            Ok(response)
        })
    }
}

@songww
Copy link
Contributor Author

songww commented Aug 3, 2021

It would be better to be able to automatically discard the response body on HEAD handler.

@davidpdrsn
Copy link
Member

davidpdrsn commented Aug 3, 2021

It would be better to be able to automatically discard the response body on HEAD handler.

Does any other frameworks do this? Doesn't seem like Rocket does

Rocket won't interfere with HEAD requests your application explicitly handles.

Mdn makes it pretty clear that dropping response bodies on HEAD automatically is the way to go though. See https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/HEAD

Edit: Also see https://httpwg.org/specs/rfc7231.html#HEAD: "The HEAD method is identical to GET except that the server MUST NOT send a message body in the response"

@jplatte
Copy link
Member

jplatte commented Aug 3, 2021

I thing especially for files the right solution would be to provide some way to support both methods at once, but with a response-type specific implementation. For example a plain file serving route would ideally use sendfile internally for GET and stat for HEAD. Do the ServeDir and ServeFile services from tower-http already do this (seems like you don't register those with a specific method but I can't see any method-specific code in ServeFiles code)?

@davidpdrsn
Copy link
Member

For example a plain file serving route would ideally use sendfile internally for GET and stat for HEAD

Are those linux syscalls or something? I'm not familiar with them. ServeDir and ServeFile both returns a tokio::fs::File (mapped to an http_body::Body). And they don't change their behavior based on the http method. Arguably that was an oversight on my part 🤷

@jplatte
Copy link
Member

jplatte commented Aug 3, 2021

Are those linux syscalls or something?

They're C functions probably translating directly to syscalls, yes. stat's Rust equivalent is std::fs::metadata, but sendfile doesn't have one. It copies data from one file descriptor to another within the kernel, and should be even more efficient than mmaping a file and sending its bytes in userspace code because it minimizes context switching between application and kernel. I think I've heard that it is the main reason serving static files with a server like Nginx / Apache can be way faster than a naive implementation.

And they don't change their behavior based on the http method. Arguably that was an oversight on my part 🤷

I've opened a separate issue for that: tower-rs/tower-http#113

@davidpdrsn davidpdrsn added the I-needs-decision Issues in need of decision. label Aug 3, 2021
@davidpdrsn davidpdrsn added this to the 0.2 milestone Aug 4, 2021
@davidpdrsn davidpdrsn removed the I-needs-decision Issues in need of decision. label Aug 5, 2021
@xmo-odoo
Copy link

xmo-odoo commented Jul 15, 2022

It would be better to be able to automatically discard the response body on HEAD handler.

Does any other frameworks do this? Doesn't seem like Rocket does

In Python land, Werkzeug (and thus Flask which is built on it) straight up refuse routing HEAD, because of its official semantics it's considered a "special" method, and a HEAD request is always routed to the GET handler, then the body is dropped during response. It's still possible to code against it by checking the request method inside the handler, but not at the routing layer. See this comment by the maintainer on a recent issue.

In the same ecosystem, mod_wsgi also has special handling for this, see this essay by the author for the concerns about it. By default mod_wsgi will may automatically remap HEAD to GET depending on Apache's output filters (e.g. if content-based caching is enabled, such that the response headers of HEAD are correct), although release 4.3.0 (2014) added an option (the default behaviour remains that above, but it can also be enabled always or never).

@davidpdrsn
Copy link
Member

Being able to add routes for HEAD requests still feels like a good idea to me. Then you can avoid generating the body since it'll be stripped anyway.

@jplatte
Copy link
Member

jplatte commented Jul 19, 2022

I'm not sure it's wise to disallow separate HEAD handlers, but I do think that using an http::Method extractor for GET-or-HEAD handlers and using its value to decide whether to include a body in the response (esp. if getting the body is expensive) is a good pattern, maybe we should add that to the docs as a suggestion, or have it in one of the examples?

@davidpdrsn
Copy link
Member

Adding an example for it sounds good!

lexcao added a commit to lexcao/axum that referenced this issue Jul 23, 2022
As tokio-rs#84 add get-head request, adding an example.
lexcao added a commit to lexcao/axum that referenced this issue Jul 24, 2022
As tokio-rs#84 add get-head request, adding an example.
lexcao added a commit to lexcao/axum that referenced this issue Jul 25, 2022
As tokio-rs#84 add get-head request, adding an example.
lexcao added a commit to lexcao/axum that referenced this issue Jul 26, 2022
As tokio-rs#84 add get-head request, adding an example.
lexcao added a commit to lexcao/axum that referenced this issue Jul 26, 2022
As tokio-rs#84 add get-head request, adding an example.
lexcao added a commit to lexcao/axum that referenced this issue Jul 26, 2022
As tokio-rs#84 add get-head request, adding an example.
lexcao added a commit to lexcao/axum that referenced this issue Jul 26, 2022
As tokio-rs#84 add get-head request, adding an example.
davidpdrsn pushed a commit that referenced this issue Jul 26, 2022
As #84 add get-head request, adding an example.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: A PR with an enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants