Proposal: introduce actix-web to simplify HTTP handling?

[siddontang]

Feature Request

Is your feature request related to a problem? Please describe:

Refer to #4444

Seem that we may add more and more HTTP APIs so the origin hyper can't fit our need.

@BusyJay and @breeswish think https://github.com/actix/actix-web is a better choice.

Describe alternatives you've considered:

Maybe compare other Web frameworks.

[nrc]

Actix is a decent library, but it has downsides: it uses non-standard futures (which may have soundness issues), it is a pretty opinionated, heavyweight framework, and it is large and complex. We should definitely compare other web frameworks to make sure we pick the right one; we should at least consider Rocket and Tide. It would also be good to know exactly how Hyper is not meeting our needs - I had a quick look at #4444 but it wasn't obvious what Actix offers which Hyper doesn't.

[breezewish]

@nrc We mostly want a framework with some better routing features, for example, able to match path parameters. In future we will also want HTTP body parsing and query parameter parsing. As you can see in #4444 we did path parameter matching manually which is awful. Additionally we will add more and more debug features or control features to this API endpoint in future so that using a pleasant and easy-to-organize framework is pretty important. According to techempower benchmark actix seems to be a well-balanced choice between our functionality demand and performance.

[jswh]

The Rocket is quite an application framework which is not a good choice for intergration.

[liufuyang]

Yes, I also feels Rocket is a bit too large. Tide is new and aiming for using standard futures I remember. I think it was relatively small sometime ago but I can see it keeps on growing. (Or perhaps the growing of it will not be an issue...)

Anyway I think Tide have very nice api designs:
https://github.com/rustasync/tide/blob/master/examples/messages.rs

async fn get_message(cx: Context<Database>) -> EndpointResult {
    let id = cx.param("id").client_err()?;
    if let Some(msg) = cx.state().get(id) {
        Ok(response::json(msg))
    } else {
        Err(StatusCode::NOT_FOUND)?
    }
}

fn main() {
    let mut app = App::with_state(Database::default());
    app.at("/message/:id").get(get_message).post(set_message);
    app.run("127.0.0.1:8000").unwrap();
}

What do you think? @breeswish have you also looked into Tide? However it is very new and marked as not production ready, and perhaps less people using it in production.

[breezewish]

@liufuyang I don't have experience with Tide, but your example looks to be pretty attractive. My concern is that it is marked as not production ready and I don't know why it is not ready :)

[liufuyang]

yeah... perhaps it uses many unstable features such as futures-preview = "0.3.0-alpha.16"? Perhaps I can try ask in the workgroup about it.

[liufuyang]

As I can see for now we only need to add a simple config endpoint to handle HTTP GET calls, perhaps we can just use current hyper solution, and we can wait a bit on using a web framework now? As soon the standard future will be ready (I hope), then many things could be changed and perhaps it will be easier to select a web framework (or even customise a small web server later on, or implement the feature we need directly in hyper?). I feel if Rust can do the async IO well with awesome build-in futures, things could look quite different by then.

[breezewish]

@liufuyang You can continue working on switching to (a stable) framework and later change to some better ones when it is ready.

[jlerche]

Here's the actix-web issue for releasing 1.0 actix/actix-web#722 and there's some discussion about futures in there. The goal I think is to transition to futures 0.3 when tokio does.

I think one big plus for actix-web is that its development is very active and was recently released as 1.0.

[nrc]

just use current hyper solution, and we can wait a bit on using a web framework now?

I would encourage this - I don't think any of the current solutions are great and there is a high risk of getting trapped into one. Perhaps it is possible to write a routing utility specific to our needs which is not too complicated?

marked as not production ready and I don't know why it is not ready

by understanding is that it is just at an early stage of development and needs iteration, rather than there being any specific issue.

The Rocket is quite an application framework which is not a good choice for intergration.

I agree, and also feel the same way about Actix.

Anyway I think Tide have very nice api designs:

Yes! It has the best API IMO and also sticking very closely to standard Rust conventions/patterns/libraries.

yeah... perhaps it uses many unstable features such as futures-preview = "0.3.0-alpha.16"

Note that futures 0.3 is the version of futures which will become 1.0 futures and everyone not using it will have to upgrade at some point (including us!).

[liufuyang]

@breeswish What do you think, should we wait a bit on this? And before that we can now just add a few more endpoints for getting configs? Correct me if I am wrong but for now I think adding just a few more config showing endpoints sounds something could be very useful and beside that we don't have clear plans on adding more endpoints for other purpose? 🤔

[breezewish]

Switching to a better routing facility / framework is just a remaining task of previous failpoint PRs. It does not have strong relationship with future changes. @liufuyang

[liufuyang]

@breeswish Yes, I agree it's good to have some better routing. What's your take on nrc's input? Do you think he worries too much and we should just give actix-web a try now or? I hope an agreement can be made soon so @jlerche and I can try start working on related stuff :)

[siddontang]

em, I think we waste too many time on this, introducing an HTTP framework has a low risk because it is not in the critical path. we can try anyone if we think it is good to use.

If now we can using hyper can solve our problem, let's keep the same, if not, let's decide one and go on.

@breeswish @jlerche @nrc @liufuyang can you make a decision this week?

/cc @BusyJay

[siddontang]

If you don't reach an agreement, I think we can let @liufuyang choose one solution and advance it. If he chooses one framework but we have to use another later, I also think he is able to do the migration.

[BusyJay]

I can see that no great solution is available at the moment. I suggest to keep current implementation and wait a little bit to see what happen next.

[liufuyang]

Thanks for the input above. So how about this, we (@jlerche and me) will start on adding the config endpoint with the current hyper way. Then at the same time I will see whether I can do some refactory (perhaps making current status server a standalone package module?) and also at the same time try out actix-web or tide, and see whether the build time will get any influence and so on.
Then later if the code looks easy to change to another web framework, then perhaps we can ship with actix-web or tide directly then continue polishing it if necessary.
@breeswish @siddontang How do you think about this idea? I can promise you I will try to make the status server part looks better, hopefully before the end of this year 😄

[breezewish]

I feel fine, since status server is not in our critical path and future change does not heavily depend on the framework change.

[jlerche]

I have no problem continuing with hyper.

I am personally biased towards actix, however, as I have been using it for quite a while, so it gets my vote when the time comes 😀

[liufuyang]

Thanks, sounds good. @jlerche you can start adding those config list endpoints now I think, just make a PR and let take a look at it when you get it working :) You can ping me or others on the community slack for help. I am new to tikv repo as well but we can learn it together :)

[nrc]

sgtm!

[Hoverbear]

Related to #4997

[liufuyang]

Maybe we can close this issue now ...

[siddontang]

Feeling sad for actix-web. 😭