----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53487/#review155508 -----------------------------------------------------------
Mostly some questions here about whether we can simplify the structure of visit. 3rdparty/libprocess/src/process.cpp (lines 673 - 675) <https://reviews.apache.org/r/53487/#comment225479> How about also describing the current semantics? Namely, the caller should ensure the provided request is valid until the future transitions. 3rdparty/libprocess/src/process.cpp (lines 682 - 684) <https://reviews.apache.org/r/53487/#comment225481> I assume you used to pull this out to take a non-const copy? Seems like this could become: ``` return pipeRequest->reader->readAll() .then(...); ``` 3rdparty/libprocess/src/process.cpp (lines 2707 - 2766) <https://reviews.apache.org/r/53487/#comment225486> 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. 3rdparty/libprocess/src/process.cpp (lines 2711 - 2712) <https://reviews.apache.org/r/53487/#comment225488> Maybe a little comment here that we're guaranteed that for a given socket, the request's body consumption continuation will run before the next request arrives in handle. We rely on this, because otherwise requests could be processed out-of-order! 3rdparty/libprocess/src/process.cpp (line 2712) <https://reviews.apache.org/r/53487/#comment225487> What are you capturing here? Is it too onerous to be explicit? 3rdparty/libprocess/src/process.cpp (lines 2757 - 2759) <https://reviews.apache.org/r/53487/#comment225484> Yikes, if the message returned from parse is null we're not sending an error response? Looking at the error handling you added above, it looks like this case also needs to be handled? 3rdparty/libprocess/src/process.cpp (lines 3611 - 3702) <https://reviews.apache.org/r/53487/#comment225492> 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 :) 3rdparty/libprocess/src/process.cpp (line 3669) <https://reviews.apache.org/r/53487/#comment225493> We should consume the body in this case, since the intention of pipe is to provide backpressure in the case that the reader cannot keep up with the writer (much like unix pipes). 3rdparty/libprocess/src/process.cpp (lines 3705 - 3710) <https://reviews.apache.org/r/53487/#comment225500> 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. ``` - Benjamin Mahler On Nov. 8, 2016, 8:23 p.m., Anand Mazumdar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53487/ > ----------------------------------------------------------- > > (Updated Nov. 8, 2016, 8:23 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 > >
