Skip to content

implement Endpoint trait for Box<dyn Endpoint>#710

Merged
yoshuawuyts merged 1 commit intohttp-rs:mainfrom
mendelt:boxed_endpoint
Sep 28, 2020
Merged

implement Endpoint trait for Box<dyn Endpoint>#710
yoshuawuyts merged 1 commit intohttp-rs:mainfrom
mendelt:boxed_endpoint

Conversation

@mendelt
Copy link
Contributor

@mendelt mendelt commented Sep 28, 2020

Right now adding endpoints dynamically is difficult because only types that implement the Endpoint trait directly can be added. When you don't know in advance which endpoint you want to add in a certain situation it can be useful to be able to add boxed endpoints. This implementation makes this possible.

I did not add any tests for this. It seems like a trivial piece of code where the test will probably be more fragile than the code itself.

@yoshuawuyts yoshuawuyts merged commit fd6ba06 into http-rs:main Sep 28, 2020
@hasali19
Copy link

hasali19 commented Oct 1, 2020

I just noticed this while going through the code for an unrelated reason. I'm still a bit new to rust but won't this cause an infinite recursion. For example:

fn endpoint() -> Box<dyn tide::Endpoint<()>> {
    Box::new(|_| async { Ok("") })
}

#[async_std::main]
async fn main() {
    let mut app = tide::Server::new();

    app.at("/").get(endpoint());

    let _: tide::http::Response = app
        .respond(tide::http::Request::new(
            tide::http::Method::Get,
            tide::http::Url::parse("http://example.com/").unwrap(),
        ))
        .await
        .unwrap();
}

This code causes a stack overflow.

@mendelt
Copy link
Contributor Author

mendelt commented Oct 1, 2020

Hmm yeah, good catch! I'll see if I can fix that!
thanks!

@mendelt
Copy link
Contributor Author

mendelt commented Oct 1, 2020

Ok, this was stupid of me. I forgot to add as_ref in the method where the boxed endpoint should call the endpoint inside the box. So it called itself. This should be fixed in this pr #711

Thanks again @hasali19 for noticing this.

@mendelt mendelt deleted the boxed_endpoint branch December 21, 2020 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants