mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 54115: Added an http::serve abstraction.
Date Tue, 29 Nov 2016 07:52:57 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54115/#review157206
-----------------------------------------------------------




3rdparty/libprocess/include/process/http.hpp (lines 884 - 885)
<https://reviews.apache.org/r/54115/#comment227663>

    I'm not sure you should mention 'Connection: Keep-Alive' since that is from HTTP 1.0.
HTTP 1.1 has persistent connections by default unless 'Connection: close' is specified.



3rdparty/libprocess/include/process/http.hpp (line 886)
<https://reviews.apache.org/r/54115/#comment227665>

    Can you comment a bit on what discarding will do? i.e. is it a clean shutdown or not?
    
    Because of this, I'm a bit inclined to think that we'll need an http::Server which can
take arguments for this kind of thing. As an example:
    
    ```
    class Server
    {
      struct ShutdownOptions
      {
        // During the grace period, no new connections will
        // be accepted. Existing connections will be closed
        // when currently received requests have been handled.
        // The server will shut down reads on each connection
        // to prevent new requests from arriving.
        Duration gracePeriod;
      };
    
      // Shuts down the server.
      Future<Nothing> shutdown(Sever::ShutdownOptions options);
    };
    ```
    
    Since you're planning to remove the serve function below, how about leaving a TODO so
that others understand the thinking? I'm thinking we would probably expose the 'Server' to
callers and hide the 'serve' function from them (as it seems most callers will want the server).



3rdparty/libprocess/src/http.cpp (line 1398)
<https://reviews.apache.org/r/54115/#comment227678>

    Socket::send is const? Weird. :)



3rdparty/libprocess/src/http.cpp (line 1432)
<https://reviews.apache.org/r/54115/#comment227679>

    A bit weird that this takes a const socket, given it's going to do write operations on
it.



3rdparty/libprocess/src/http.cpp (line 1433)
<https://reviews.apache.org/r/54115/#comment227677>

    Why the copy? Note that copies of requests/responses is expensive, we want to move towards
0 copying of them.



3rdparty/libprocess/src/http.cpp (lines 1436 - 1437)
<https://reviews.apache.org/r/54115/#comment227682>

    This seems a bit brittle since we're not checking for the things we expect, but rather
what we do not expect. Should you just check for NONE and `BODY` here? I'm actually not sure
why we have `NONE`, it seems equivalent to `BODY` with `body.empty()`.



3rdparty/libprocess/src/http.cpp (line 1449)
<https://reviews.apache.org/r/54115/#comment227680>

    Ditto here.



3rdparty/libprocess/src/http.cpp (line 1450)
<https://reviews.apache.org/r/54115/#comment227681>

    Ditto here w.r.t. copying.



3rdparty/libprocess/src/http.cpp (lines 1455 - 1457)
<https://reviews.apache.org/r/54115/#comment227683>

    Should we CHECK against this since you're not handling the programming error?



3rdparty/libprocess/src/http.cpp (lines 1455 - 1457)
<https://reviews.apache.org/r/54115/#comment227684>

    Should we CHECK against this since you're not handling the programming error? I'm not
sure how we could surface this to the handler writer, it seems the only thing we could do
is to CHECK here or prevent this kind of mal-construction altogether.



3rdparty/libprocess/src/http.cpp (line 1459)
<https://reviews.apache.org/r/54115/#comment227685>

    This FD will leak into the child processes since you don't have `O_CLOEXEC`. Also, don't
you want `O_NONBLOCK` here to avoid blocking in the kernel during reads?
    
    We should use `os::open` from stout here, which handles O_NONBLOCK on systems without
it defined:
    
    ```
    Try<int> fd = os::open(response.path, O_CLOEXEC | O_NONBLOCK | O_RDONLY);
    ```
    
    And you won't have to use os::strerror.



3rdparty/libprocess/src/http.cpp (lines 1469 - 1483)
<https://reviews.apache.org/r/54115/#comment227686>

    Hm.. seems like we need an `os::isdir` overload that takes the fd:
    
    ```
    bool os::isdir(const string& path); // exists
    bool os::isdir(int fd); // does not exist
    ```
    
    oh well



3rdparty/libprocess/src/http.cpp (line 1476)
<https://reviews.apache.org/r/54115/#comment227688>

    You're leaking the fd here?



3rdparty/libprocess/src/http.cpp (line 1482)
<https://reviews.apache.org/r/54115/#comment227689>

    You're leaking the fd here?



3rdparty/libprocess/src/http.cpp (lines 1493 - 1504)
<https://reviews.apache.org/r/54115/#comment227691>

    If the .then doesn't run, it looks like the fd will be leaked.



3rdparty/libprocess/src/http.cpp (line 1498)
<https://reviews.apache.org/r/54115/#comment227687>

    Indentation is off here.



3rdparty/libprocess/src/http.cpp (line 1499)
<https://reviews.apache.org/r/54115/#comment227690>

    It's weird that you value capture encoder only to reassign it, can you just avoid the
value capture of encoder here?



3rdparty/libprocess/src/http.cpp (line 1539)
<https://reviews.apache.org/r/54115/#comment227692>

    It looks like there is a bug here introduced in the conversion from process.cpp.
    
    It doesn't matter if you call reader.close in the EOF path, that's harmless. But, it's
critical to call reader.close() in the case that we encounter an error here (i.e. if `send`
fails). This code does not do this.
    
    It would be cleanest to have a `reader.close()` in an always executing path once we're
done the read loop (either successfully or due to failure).



3rdparty/libprocess/src/http.cpp (line 1548)
<https://reviews.apache.org/r/54115/#comment227694>

    Ditto const weirdness.



3rdparty/libprocess/src/http.cpp (line 1549)
<https://reviews.apache.org/r/54115/#comment227693>

    Ditto copying here.



3rdparty/libprocess/src/http.cpp (lines 1554 - 1556)
<https://reviews.apache.org/r/54115/#comment227695>

    Ditto my other comment about body being set here.



3rdparty/libprocess/src/http.cpp (lines 1558 - 1566)
<https://reviews.apache.org/r/54115/#comment227696>

    Hm.. while it's harsh, I imagine the user is going to have to hunt down this bug in their
code if we take this approach. I'd rather CHECK here that they're not invalidating the invariants,
otherwise the bug can slip in silently. =/



3rdparty/libprocess/src/http.cpp (lines 1574 - 1580)
<https://reviews.apache.org/r/54115/#comment227697>

    We need to close the reader in the error path here if the send fails or discards.



3rdparty/libprocess/src/http.cpp (lines 1608 - 1611)
<https://reviews.apache.org/r/54115/#comment227700>

    Maybe comment that we're currently not making any discard requests in this code so we
don't expect this? I would expect that we'll start making discard requests once we use something
like `process::Stream` (see my other comment) and can be notified of the write-end of the
stream failing.



3rdparty/libprocess/src/http.cpp (lines 1632 - 1640)
<https://reviews.apache.org/r/54115/#comment227702>

    Note that if we were to use `process::Stream<Item>`, the read end of the stream
would close here and the write-end (`receive()`) could detect this and stop further receiving
(right now you accomplish this with a read shutdown up in the caller). Also, ideally we discard
the remaining handler futures at this point.
    
    Maybe a TODO here that we should discard the remaining handlers when there is a 'connection:
close'?



3rdparty/libprocess/src/http.cpp (line 1636)
<https://reviews.apache.org/r/54115/#comment227701>

    ```
    s/response.headers.get("Connection").get()/response.headers.at("Connection")/
    ```



3rdparty/libprocess/src/http.cpp (lines 1712 - 1717)
<https://reviews.apache.org/r/54115/#comment227699>

    We should have a TODO to use `process::Stream` here which would be the typed version of
`process::Pipe` (i.e. `process::Pipe<Item_Type>`) since it offers us the semantics we
want:
    
    (1) Stream completion (i.e. EOF).
    (2) Backpressure (I haven't implemented this for http::Pipe yet but the idea was to add
asynchronous backpressure. In this case if the receiving of data is too fast for the handler
code then the backpressure would slow down our reading and apply TCP backpressure).
    (3) Notification of the writer failing (In this case it will allow us to discard the handler
futures if the socket disconnects).
    
    I have a few TODOs around related to process::Stream. Also, I'm not sure if we'll want
process::Queue if we have process::Stream.



3rdparty/libprocess/src/http.cpp (lines 1751 - 1752)
<https://reviews.apache.org/r/54115/#comment227698>

    Hm.. I don't find this very intuitive, in terms of why the best course of action is to
shut down only the read end, we can't send either, shouldn't we fully disconnect the socket
by shutting down reads and writes?



3rdparty/libprocess/src/tests/http_tests.cpp (line 1766)
<https://reviews.apache.org/r/54115/#comment227670>

    A lot of this test seems devoted to testing pipelining, how about testing pipelining in
its own test, and having a smaller test for unix domain sockets?
    
    And maybe a short comment at the top?
    
    ```
    // Ensures that `http::serve()` respects pipelining.
    TEST(HttpServeTest, Pipelining)
    
    // Ensures we can serve HTTP from a domain socket.
    TEST(HttpServeTest, Unix)
    ```



3rdparty/libprocess/src/tests/http_tests.cpp (lines 1778 - 1781)
<https://reviews.apache.org/r/54115/#comment227667>

    How about:
    
    ```
      Future<http::Connection> connection = http::connect(address);
      AWAIT_READY(connection);
      
      // And just use the -> operator below:
      // connection->send(...);
      // connection->disconnect();
    ```



3rdparty/libprocess/src/tests/http_tests.cpp (line 1830)
<https://reviews.apache.org/r/54115/#comment227673>

    Maybe just "3", "1", "2" instead of "bar", "world", "foo"? Seems easier to follow the
intention this way?



3rdparty/libprocess/src/tests/http_tests.cpp (lines 1852 - 1854)
<https://reviews.apache.org/r/54115/#comment227675>

    Whoops?


- Benjamin Mahler


On Nov. 28, 2016, 6:20 a.m., Benjamin Hindman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54115/
> -----------------------------------------------------------
> 
> (Updated Nov. 28, 2016, 6:20 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added an http::serve abstraction.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/http.hpp a684e09c8353112a0656b7e899a469c1e022e93b

>   3rdparty/libprocess/src/http.cpp 3f16f293a5c5cd0b31a85efe94cb6f8019543d45 
>   3rdparty/libprocess/src/tests/http_tests.cpp d41929a9c8b2469c10b9e31985c447076c1684dc

> 
> Diff: https://reviews.apache.org/r/54115/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message