----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61155/#review181783 -----------------------------------------------------------
A pretty clean http server! I held off on a ship it because there were a couple of issues that we should discuss. 3rdparty/libprocess/include/process/http.hpp Line 1020 (original), 1020-1022 (patched) <https://reviews.apache.org/r/61155/#comment257505> Fix this separately? 3rdparty/libprocess/include/process/http.hpp Lines 1047-1050 (patched) <https://reviews.apache.org/r/61155/#comment257514> FYI I think this might not work for windows 3rdparty/libprocess/include/process/http.hpp Lines 1059 (patched) <https://reviews.apache.org/r/61155/#comment257506> Issuing a shutdown of reads doesn't seem quite right to me, it will cut off any requests that we haven't fully read yet and cut off any pipelined requests that are behind it already. Even if shutdown is guaranteed to preserve the data in the receive buffer we'll still be cutting off any requests that don't completely reside within the receive buffer? I would expect that graceful shutdown will at least finish reading any active requests, and ideally (to gracefully handle pipelined requests) keep reading until reading is blocked and no requests are being handled. As an aside, apparently (since not specified by POSIX) the behavior when clients send more data after a shutdown(SHUT_RD) is platform dependent: ``` * Windows will issue an RST which eventually gives the sender a 'connection reset'. * BSD-based Unixes will accept and ignore the data (i.e. ACK it but throw it away, rather than entering it into your socket receive buffer), so the sender will just keep sending as much as he likes, until you close the socket, when he will either read an end-of-stream or get a 'connection reset' on write. * Linux will accept the data until your socket send buffer fills up; then your receive window will close, which blocks the sender; then when you close the socket the sender will get a 'connection reset'. ``` E.g. this was from https://stackoverflow.com/a/30317014 3rdparty/libprocess/include/process/http.hpp Lines 1071 (patched) <https://reviews.apache.org/r/61155/#comment257515> FYI I think this might not work for windows 3rdparty/libprocess/include/process/http.hpp Lines 1076 (patched) <https://reviews.apache.org/r/61155/#comment257507> Just curious, because it's not obvious to me, when does the caller need to pass a socket instead of the address? 3rdparty/libprocess/include/process/http.hpp Lines 1124-1130 (patched) <https://reviews.apache.org/r/61155/#comment257510> stop seems to suggest a s/run/start/? 3rdparty/libprocess/include/process/http.hpp Lines 1132-1133 (patched) <https://reviews.apache.org/r/61155/#comment257511> I suspect callers will want something synchronous here since the address should already be known, you could do something similar to connection: https://github.com/apache/mesos/blob/f8859b9f6bec0a21584000508d1fbf77aa18ec62/3rdparty/libprocess/include/process/http.hpp#L963-L964 i.e. store the address in Server since you always have it in hand when you're constructing the Server object. 3rdparty/libprocess/include/process/http.hpp Lines 1143 (patched) <https://reviews.apache.org/r/61155/#comment257520> Any reason not to store a `PID<ServerProcess>` and spawn with `managed=true`? 3rdparty/libprocess/src/http.cpp Line 1404 (original), 1406-1407 (patched) <https://reviews.apache.org/r/61155/#comment257512> whoops? 3rdparty/libprocess/src/http.cpp Lines 1705-1713 (original), 1708-1730 (patched) <https://reviews.apache.org/r/61155/#comment257513> whoops? 3rdparty/libprocess/src/http.cpp Lines 1964-1971 (patched) <https://reviews.apache.org/r/61155/#comment257516> FYI I think this might not work for windows 3rdparty/libprocess/src/http.cpp Lines 1973-1978 (patched) <https://reviews.apache.org/r/61155/#comment257518> Hm.. can you avoid the copy here and instead move the client into the clients map? 3rdparty/libprocess/src/http.cpp Lines 1987 (patched) <https://reviews.apache.org/r/61155/#comment257521> It doesn't look like you need the `-> Future<Nothing>` here given both returns have the same type? 3rdparty/libprocess/src/http.cpp Lines 1997-2003 (patched) <https://reviews.apache.org/r/61155/#comment257524> Hm.. isn't this only the DISCARDED case? In the failure case it seems like that's a problem with the accept loop. Maybe the logic should be: ``` if discarded: check stopping return stopping return shutdown(failure(f.condition)) ``` 3rdparty/libprocess/src/http.cpp Lines 2013-2021 (patched) <https://reviews.apache.org/r/61155/#comment257525> Seems like we should reverse these? i.e. if stopping is some return that, then if we were running but are not anymore, return nothing because we're already stopped? As it stands, it seems odd to return the running future within stop, you would return running's failure / discarded information? (e.g. "Failed to run: xxx"). 3rdparty/libprocess/src/http.cpp Lines 2027 (patched) <https://reviews.apache.org/r/61155/#comment257526> whereas 3rdparty/libprocess/src/http.cpp Lines 2034 (patched) <https://reviews.apache.org/r/61155/#comment257527> s/ / / s/bypass/bypasses/ ? s/./)./ 3rdparty/libprocess/src/http.cpp Lines 2039-2045 (patched) <https://reviews.apache.org/r/61155/#comment257528> See my comment above about why this doesn't have the graceful semantics we want IMO 3rdparty/libprocess/src/http.cpp Lines 2047-2053 (patched) <https://reviews.apache.org/r/61155/#comment257530> This doesn't seem right? This will always wait the full grace period to satisfy the stop future, even if we stop early? 3rdparty/libprocess/src/http.cpp Lines 2064-2067 (patched) <https://reviews.apache.org/r/61155/#comment257523> Why do we need to initiate a stop here? And why stop instead of shutdown? 3rdparty/libprocess/src/http.cpp Lines 2085-2086 (patched) <https://reviews.apache.org/r/61155/#comment257529> Why a `shutdown(READ_WRITE)` in stop but no `shutdown(READ_WRITE)` in shutdown? 3rdparty/libprocess/src/http.cpp Lines 2087 (patched) <https://reviews.apache.org/r/61155/#comment257531> Hm.. don't we need to immediately discard the client servings? Otherwise you might not be able to defer back into the ServerProcess before it terminates? Seems also like at this point we need to prevent the accept loop from accepting more clients? 3rdparty/libprocess/src/http.cpp Lines 2112 (patched) <https://reviews.apache.org/r/61155/#comment257522> Maybe 'handler' instead of 'f' in all of these places? I think users will already be thinking of it as the "request handler". 3rdparty/libprocess/src/http.cpp Lines 2177-2178 (patched) <https://reviews.apache.org/r/61155/#comment257532> Can you open and close the quotes on the same line? - Benjamin Mahler On July 27, 2017, 1:55 a.m., Benjamin Hindman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61155/ > ----------------------------------------------------------- > > (Updated July 27, 2017, 1:55 a.m.) > > > Review request for mesos and Benjamin Mahler. > > > Repository: mesos > > > Description > ------- > > Added http::Server. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/http.hpp > f637999174d92a98208b5fc49a65f9929efb11a0 > 3rdparty/libprocess/src/http.cpp a4d71fb6c345d3c7a7611004830f6c2c0fbf6046 > 3rdparty/libprocess/src/tests/http_tests.cpp > dde05f6a554fcb8c6c89e690bbdcd2bf509854d5 > > > Diff: https://reviews.apache.org/r/61155/diff/1/ > > > Testing > ------- > > make check > > > Thanks, > > Benjamin Hindman > >
