[Merged by Bors] - Shift HTTP server heavy-lifting to blocking executor #1518
Conversation
0e39b2d to
343ecc8
Compare
| beacon_chain: Arc<BeaconChain<T>>, | ||
| ) -> ApiResult { | ||
| req: Request<Vec<u8>>, | ||
| ctx: Arc<Context<T>>, |
There was a problem hiding this comment.
I'd advise against using a one big Context type. It may make writing request handlers less of a hassle, but most of the fields in it are unused and if a handler was to be used from outside of this particular module, the caller would have to construct the entire Context type, instead of passing what's actually gets used: i.e. BeaconChain in this case.
| ) -> ApiResult { | ||
| req: Request<Vec<u8>>, | ||
| ctx: Arc<Context<T>>, | ||
| ) -> Result<Hash256, ApiError> { |
There was a problem hiding this comment.
Rust books recommend having pub type Result<T> = std::result::Result<T, ApiError> alias at the top of a module and use something like Result<Hash256> as the function return type. It's easier to use and there's less repetition because error types tend to stay the same in a single module.
| "The request body must be empty".to_string(), | ||
| )) | ||
| } else { | ||
| Ok(bytes.into_iter().collect()) |
There was a problem hiding this comment.
What's the reason for converting Bytes into Vec<u8>? Is there some functionality that Bytes doesn't support? Preferring Bytes should generally be more memory-efficient.
| where | ||
| F: Fn(Request<()>, T) -> Result<Body, ApiError>, | ||
| { | ||
| let body = func(self.req, self.ctx)?; |
There was a problem hiding this comment.
This callback style is hiding away the OS thread creation. The plan is to have a single OS thread shared among several (SSE) HTTP endpoints, so I think it should be brought into forefront. Also, instead of passing around requests/responses as function arguments/return values, I'd suggest passing around synchronization primitives (channel, bus). Requests/responses do not reflect the actual program control flow in case of hyper's Body::channel().
|
Thanks for the thoughts @adaszko, some very good ones I'll keep in mind for the upcoming API refactor. For the time being, I'll just go ahead with what we have since the comments are either performance-based or stylistic. I'm concerned about spending too much time on this when it's going to get refactored. bors r+ |
## Issue Addressed NA ## Proposed Changes Shift practically all HTTP endpoint handlers to the blocking executor (some very light tasks are left on the core executor). ## Additional Info This PR covers the `rest_api` which will soon be refactored to suit the standard API. As such, I've cut a few corners and left some existing issues open in this patch. What I have done here should leave the API in state that is not necessary *exactly* the same, but good enough for us to run validators with. Specifically, the number of blocking workers that can be spawned is unbounded and I have not implemented a queue; this will need to be fixed when we implement the standard API.
|
Pull request successfully merged into master. Build succeeded: |
|
👎 |
Issue Addressed
NA
Proposed Changes
Shift practically all HTTP endpoint handlers to the blocking executor (some very light tasks are left on the core executor).
Additional Info
This PR covers the
rest_apiwhich will soon be refactored to suit the standard API. As such, I've cut a few corners and left some existing issues open in this patch. What I have done here should leave the API in state that is not necessary exactly the same, but good enough for us to run validators with. Specifically, the number of blocking workers that can be spawned is unbounded and I have not implemented a queue; this will need to be fixed when we implement the standard API.