> On Nov. 9, 2016, 11:58 p.m., Benjamin Mahler wrote: > > 3rdparty/libprocess/src/process.cpp, lines 3708-3713 > > <https://reviews.apache.org/r/53487/diff/2/?file=1556799#file1556799line3708> > > > > At this point `_visit` looks like: > > > > (1) Authenticate > > (2) Authorize > > (3) Call handler > > > > Have you looked at making this chain explicit in visit? It seems much > > simpler to understand: > > > > ``` > > // Within visit: > > > > if (endpoint.isSome()) { > > authenticate(request) > > .then(authorize, request) > > .onAny(_visit, request); > > } > > > > // Then _visit would do the conversion if necessary before sending it > > to the handler. > > ```
As per our offline discussion, I modified `_visit()` to return `Future<Response>`. > On Nov. 9, 2016, 11:58 p.m., Benjamin Mahler wrote: > > 3rdparty/libprocess/src/process.cpp, lines 3614-3705 > > <https://reviews.apache.org/r/53487/diff/2/?file=1556799#file1556799line3614> > > > > How about we simplify this with resolvers? > > > > ``` > > void ProcessBase::visit(const HttpEvent& event) > > { > > VLOG(1) << "Handling HTTP event for process '" << pid.id << "'" > > << " with path: '" << event.request->url.path << "'"; > > > > // First try to resolve an endpoint for this path. > > Option<HttpEndpoint> endpoint = > > resolveEndpoint(event.request->url.path); > > > > if (endpoint.isSome()) { > > if (endpoint->options->streaming) { > > // Consume the request body on behalf of the endpoint. > > convert(request.get()) > > .onAny(defer(self(), [this, endpoint, name, request, response]( > > const Future<Nothing>& future) { > > _visit(future, endpoint, name, request, response); > > })); > > } else { > > // Let the endpoint consume the request body. > > _visit(Nothing(), endpoint, name, request, response); > > } > > > > return; > > } > > > > // Ensure the body is consumed so that no backpressure is applied > > // to the socket (ignore the content since we do not care about it). > > http::Pipe::Reader reader = event->request->reader.get(); > > reader.readAll(); > > > > // If no endpoint is found, look for an asset. > > Option<Asset> asset = resolveAsset(event.request->url.path); > > > > if (asset.isSome()) { > > OK response; > > response.type = Response::PATH; > > response.path = asset->path; > > > > // Construct the final path by appending remaining tokens. > > for (int i = 2; i < tokens.size(); i++) { > > response.path += "/" + tokens[i]; > > } > > > > // Try and determine the Content-Type from an extension. > > Option<string> extension = Path(response.path).extension(); > > > > if (extension.isSome() && asset->types.count(extension.get()) > 0) { > > response.headers["Content-Type"] = > > asset->types.at(extension.get()); > > } > > > > // TODO(benh): Use "text/plain" for assets that don't have an > > // extension or we don't have a mapping for? It might be better to > > // just let the browser guess (or do its own default). > > > > event.response->associate(response); > > > > return; > > } > > > > // We cannot serve this request! > > VLOG(1) << "Returning '404 Not Found' for" > > << " '" << event.request->url.path << "'"; > > > > event.response->associate(NotFound()); > > } > > ``` > > > > You can do this in a patch in front of this one. > > Don't copy this exactly, it won't compile :) hmm, this looks like a good suggestion though looks unrelated to this patch. As discussed offline, I would take this on as part of a separate review unrelated to this chain. > On Nov. 9, 2016, 11:58 p.m., Benjamin Mahler wrote: > > 3rdparty/libprocess/src/process.cpp, lines 2707-2769 > > <https://reviews.apache.org/r/53487/diff/2/?file=1556799#file1556799line2707> > > > > Given my error handling comment below, would the following be easier? > > > > ``` > > if (libprocess(request)) { > > parse(request) > > .onAny([=](const Future<Message*>& message) { > > if (!message.isReady()) { > > // Handle error. > > } > > > > // No longer need to care about nullptr case. > > CHECK_NOTNULL(message.get()); > > > > // Handle accepted. > > }) > > } > > ``` > > > > I'm a bit hesitant to need to do the PIPE->BODY conversion since we may > > want to make all requests have a pipe body. Parse would be the consumer of > > the request, and would look like: > > > > ``` > > Future<Message*> parse(const Request& request); > > ``` > > > > Note that while its const, we can take a non-const copy of the > > request's pipe reader and consume it and this function doesn't care if the > > request is destructed since it has a copy of the pipe reader in the > > asynchronous path. Fixed - Anand ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53487/#review155508 ----------------------------------------------------------- On Nov. 11, 2016, 7:57 p.m., Anand Mazumdar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53487/ > ----------------------------------------------------------- > > (Updated Nov. 11, 2016, 7:57 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-6466 > https://issues.apache.org/jira/browse/MESOS-6466 > > > Repository: mesos > > > Description > ------- > > Old libprocess style messages and routes not supporting request > streaming read the body from the piped reader. Otherwise, the > request is forwarded to the handler when the route supports > streaming. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/process.hpp > f389d3d3b671e301a7ac911ad87ab13289e8c82f > 3rdparty/libprocess/src/process.cpp > ab2b5a9d38a3001d6a5daa1807fecb630c4b154d > > Diff: https://reviews.apache.org/r/53487/diff/ > > > Testing > ------- > > make check (Tests are added in https://reviews.apache.org/r/53490 i.e., after > we add support to the `Connection` abstraction for request streaming) > > > Thanks, > > Anand Mazumdar > >
