Update actix-web to 1.0 #67
Conversation
This is a large change, since actix-web changed significantly. It also removes Sentry integration, because currently the Sentry integration with Actix does not support actix-web 1.0.
Cargo.toml
Outdated
|
|
||
| [dependencies] | ||
| actix = "^0.7.6" | ||
| actix-web = "^0.7.14" | ||
| actix = "*" |
There was a problem hiding this comment.
Should this be * and not a specific version?
There was a problem hiding this comment.
Ah, this is just what cargo add did by default. I'll lock it down a bit better.
| HttpResponse::Ok().body(format!( | ||
| "received headers: {:?}\n\nrequest state: {:?}\n\nclient ip: {:?}", | ||
| req.headers(), | ||
| &req.state(), | ||
| &req.app_data::<EndpointState>(), |
There was a problem hiding this comment.
I like this, it's a lot closer to what we've been used to in other (Python) frameworks :)
src/logging.rs
Outdated
| // log: slog::Logger::root(slog::Discard, slog::o!()), | ||
| // } | ||
| // } | ||
| // } |
There was a problem hiding this comment.
These look like leftovers, implem was done in call()
| let mut service = test::init_service(App::new().data(state).wrap(ResponseTimer).route( | ||
| "/", | ||
| web::get().to(|| HttpResponse::InternalServerError().finish()), | ||
| )); |
There was a problem hiding this comment.
I was wondering if we couldn't have a small helper that would take an app instance and an handler to define a service and call it with a request.
There was a problem hiding this comment.
I tried a couple variations of this, and it doesn't seem to actually make the code simpler. I couldn't find anything that I liked more than the long way around. Defining the app and handler seem to take up most of the space here.
I think you're right that this is pretty unsatisfying as is. Can you try writing what the callsite might look like for the helper you have in mind? With that in hand I could implement the helper (or feel free to do both, if you want).
I think in the interest of time, I'm going to merge this PR without the helper.
There was a problem hiding this comment.
Don't worry, I think you're right. And sometimes a helper with too many parameters is not the most elegant way...
|
bors r= @leplatrem, @jaredkerim |
67: Update actix-web to 1.0 r=leplatrem,jaredkerim a=mythmon This is a large change, since actix-web changed significantly. It also removes Sentry integration, because currently the Sentry integration with Actix does not support actix-web 1.0. The biggest change is that the geoip looks now happen on the same thread instead of a separate shared set of workers. This was a neat trick we did in the initial implementation. I know that it can be done with Actix 1.0, but I didn't want to take the time to figure out how to do it right now. I suggest we load test this implementation, and if it performs similarly to the other one, keep this simpler implementation. Co-authored-by: Mike Cooper <mythmon@gmail.com> Co-authored-by: Michael Cooper <mythmon@gmail.com>
Build succeeded |
This is a large change, since actix-web changed significantly. It also removes
Sentry integration, because currently the Sentry integration with Actix does
not support actix-web 1.0.
The biggest change is that the geoip looks now happen on the same thread instead of a separate shared set of workers. This was a neat trick we did in the initial implementation. I know that it can be done with Actix 1.0, but I didn't want to take the time to figure out how to do it right now. I suggest we load test this implementation, and if it performs similarly to the other one, keep this simpler implementation.