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 49404: Changed the SocketManager to pass "Sockets" by value.
Date Thu, 30 Jun 2016 01:51:03 GMT

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


Fix it, then Ship it!




Great cleanup, thanks!


3rdparty/libprocess/src/process.cpp (line 1392)
<https://reviews.apache.org/r/49404/#comment205391>

    Technically this changes the semantics in that if an old fd is already present we will
no longer overwrite but that would have been a bug anyway. Ideally this code checks that the
insertion happens:
    
    ```
      synchronized (mutex) {
        auto result = sockets.emplace(socket, socket);
        CHECK(result->second) << "Socket fd already present in map";
      }
    ```



3rdparty/libprocess/src/process.cpp (line 1556)
<https://reviews.apache.org/r/49404/#comment205392>

    Seems we should check that this inserted:
    
    ```
    // sockets[s] = socket.get()
    CHECK(sockets.emplace(s, socket.get())->second);
    ```
    
    Or less messy:
    
    ```
    CHECK_EQ(0u, sockets.count(s));
    sockets.emplace(s, socket.get());
    ```
    
    It's a bit unfortunate that we lost readability.
    
    Ideally we could check our invariants here for the other maps as well, feel free to punt
since we're only interested in removing the socket pointer mess.



3rdparty/libprocess/src/process.cpp (lines 1580 - 1589)
<https://reviews.apache.org/r/49404/#comment205395>

    I was initially mislead by the comment for `existing` that we made a copy for some kind
of lifetime correctness, rather than just readability to passing an argument. Feel free to
do this:
    
    ```
            // Update all the data structures that are mapped to the old
            // socket. They will now point to the new socket we are about
            // to try to connect. The old socket should no longer have any
            // references after the swap and should be closed.
            Socket existing(sockets.at(persists.at(to.address)));
            swap_implementing_socket(existing, socket.get());
    ```



3rdparty/libprocess/src/process.cpp (line 2020)
<https://reviews.apache.org/r/49404/#comment205396>

    This should now be `socket.get()` because the intention is to print the fd. This works
for you because of the int cast operator, but in the future if we added a different operator
for printing sockets, the output here would silently change.



3rdparty/libprocess/src/process.cpp (line 2103)
<https://reviews.apache.org/r/49404/#comment205397>

    Ditto here for `socket.get()`.


- Benjamin Mahler


On June 30, 2016, 1:11 a.m., Joseph Wu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49404/
> -----------------------------------------------------------
> 
> (Updated June 30, 2016, 1:11 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-5748
>     https://issues.apache.org/jira/browse/MESOS-5748
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> `Sockets` is already a reference-counted `shared_ptr` under the covers.
> By passing around `Sockets` by value, we avoid potentially deleting
> a reference while the same reference is in use by another function.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/src/process.cpp 9bae71246e751e491be5a989eea8aca29c9aa751 
> 
> Diff: https://reviews.apache.org/r/49404/diff/
> 
> 
> Testing
> -------
> 
> make check (OSX)
> 
> 3rdparty/libprocess/libprocess-tests --gtest_filter="ProcessRemoteLinkTest.RemoteLink"
 --gtest_break_on_failure --gtest_repeat=10000
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>


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