You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Hindman <be...@berkeley.edu> on 2017/07/27 01:55:34 UTC
Review Request 61155: Added http::Server.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61155/
-----------------------------------------------------------
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
Re: Review Request 61155: Added http::Server.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61155/#review191471
-----------------------------------------------------------
Fix it, then Ship it!
3rdparty/libprocess/src/http.cpp
Lines 2096 (patched)
<https://reviews.apache.org/r/61155/#comment269312>
The run loop will erase the clients as they transition, was this required?
3rdparty/libprocess/src/http.cpp
Lines 2098-2100 (patched)
<https://reviews.apache.org/r/61155/#comment269311>
3rdparty/libprocess/src/tests/http_tests.cpp
Lines 2337 (patched)
<https://reviews.apache.org/r/61155/#comment269314>
Maybe a little comment about what this tests? E.g. ensures that the server does not re-order responses if handlers complete the responses out of order?
3rdparty/libprocess/src/tests/http_tests.cpp
Lines 2355-2360 (patched)
<https://reviews.apache.org/r/61155/#comment269313>
Hopefully this will become synchronous with your update?
3rdparty/libprocess/src/tests/http_tests.cpp
Lines 2419-2423 (patched)
<https://reviews.apache.org/r/61155/#comment269315>
Hm.. this doesn't make sure that 2 arrives before 3, wonder if there is an easy way to check that?
3rdparty/libprocess/src/tests/http_tests.cpp
Lines 2436 (patched)
<https://reviews.apache.org/r/61155/#comment269316>
StopNotRunning?
- 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 ba1b086a1dba140df491387077723d5c359acbcc
> 3rdparty/libprocess/src/http.cpp fa3dde68de2e543ad910febbc4c8c46309b9c147
> 3rdparty/libprocess/src/tests/http_tests.cpp ef84967a034b6293e0279b61607c07ccc2526d99
>
>
> Diff: https://reviews.apache.org/r/61155/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 61155: Added http::Server.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
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
>
>