Re: Review Request 46024: Avoided misleading locking in the libprocess SocketManager.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46024/#review128319 --- Ship it! Ship It! - Ben Mahler On April 11, 2016, 2:36 p.m., Neil Conway wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46024/ > --- > > (Updated April 11, 2016, 2:36 p.m.) > > > Review request for mesos and Ben Mahler. > > > Repository: mesos > > > Description > --- > > In `SocketManager::swap_implementing_socket`, the previous coding > accessed the `sockets` instance variable in two CHECKs without > acquiring `mutex`. This is unsafe in general: `mutex` should be > acquired before accessing instance variables like `sockets` (unless > you know all such concurrent accesses will be read-only etc.). > > As it happens, the previous coding was not unsafe because all the > call-sites of `SocketManager::swap_implementing_socket` acquire > `mutex` before invoking it. But because `swap_implementing_socket` > also acquires `mutex`, clearly its API contract allows it to be > invoked without holding `mutex`, in which case the CHECKs would > likely be unsafe. > > > Diffs > - > > 3rdparty/libprocess/src/process.cpp > 5e9dcfdc52f3a8223bc43af149b8e1f5dbdf5b0a > > Diff: https://reviews.apache.org/r/46024/diff/ > > > Testing > --- > > > Thanks, > > Neil Conway > >
Re: Review Request 46024: Avoided misleading locking in the libprocess SocketManager.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46024/#review128271 --- Mind adding joris and/or benh as reviewers? I'm happy to review but since they wrote the code I'd like them to be aware of this change. - Ben Mahler On April 11, 2016, 2:36 p.m., Neil Conway wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46024/ > --- > > (Updated April 11, 2016, 2:36 p.m.) > > > Review request for mesos and Ben Mahler. > > > Repository: mesos > > > Description > --- > > In `SocketManager::swap_implementing_socket`, the previous coding > accessed the `sockets` instance variable in two CHECKs without > acquiring `mutex`. This is unsafe in general: `mutex` should be > acquired before accessing instance variables like `sockets` (unless > you know all such concurrent accesses will be read-only etc.). > > As it happens, the previous coding was not unsafe because all the > call-sites of `SocketManager::swap_implementing_socket` acquire > `mutex` before invoking it. But because `swap_implementing_socket` > also acquires `mutex`, clearly its API contract allows it to be > invoked without holding `mutex`, in which case the CHECKs would > likely be unsafe. > > > Diffs > - > > 3rdparty/libprocess/src/process.cpp > 5e9dcfdc52f3a8223bc43af149b8e1f5dbdf5b0a > > Diff: https://reviews.apache.org/r/46024/diff/ > > > Testing > --- > > > Thanks, > > Neil Conway > >
Re: Review Request 46024: Avoided misleading locking in the libprocess SocketManager.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46024/#review128220 --- Patch looks great! Reviews applied: [46025, 46026, 46027, 46028, 46029, 46030, 46024] Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh - Mesos ReviewBot On April 11, 2016, 2:36 p.m., Neil Conway wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46024/ > --- > > (Updated April 11, 2016, 2:36 p.m.) > > > Review request for mesos and Ben Mahler. > > > Repository: mesos > > > Description > --- > > In `SocketManager::swap_implementing_socket`, the previous coding > accessed the `sockets` instance variable in two CHECKs without > acquiring `mutex`. This is unsafe in general: `mutex` should be > acquired before accessing instance variables like `sockets` (unless > you know all such concurrent accesses will be read-only etc.). > > As it happens, the previous coding was not unsafe because all the > call-sites of `SocketManager::swap_implementing_socket` acquire > `mutex` before invoking it. But because `swap_implementing_socket` > also acquires `mutex`, clearly its API contract allows it to be > invoked without holding `mutex`, in which case the CHECKs would > likely be unsafe. > > > Diffs > - > > 3rdparty/libprocess/src/process.cpp > 5e9dcfdc52f3a8223bc43af149b8e1f5dbdf5b0a > > Diff: https://reviews.apache.org/r/46024/diff/ > > > Testing > --- > > > Thanks, > > Neil Conway > >
Re: Review Request 46024: Avoided misleading locking in the libprocess SocketManager.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46024/ --- (Updated April 11, 2016, 2:36 p.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- In `SocketManager::swap_implementing_socket`, the previous coding accessed the `sockets` instance variable in two CHECKs without acquiring `mutex`. This is unsafe in general: `mutex` should be acquired before accessing instance variables like `sockets` (unless you know all such concurrent accesses will be read-only etc.). As it happens, the previous coding was not unsafe because all the call-sites of `SocketManager::swap_implementing_socket` acquire `mutex` before invoking it. But because `swap_implementing_socket` also acquires `mutex`, clearly its API contract allows it to be invoked without holding `mutex`, in which case the CHECKs would likely be unsafe. Diffs (updated) - 3rdparty/libprocess/src/process.cpp 5e9dcfdc52f3a8223bc43af149b8e1f5dbdf5b0a Diff: https://reviews.apache.org/r/46024/diff/ Testing --- Thanks, Neil Conway
Review Request 46024: Avoided misleading locking in the libprocess SocketManager.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46024/ --- Review request for mesos and Ben Mahler. Repository: mesos Description --- In `SocketManager::swap_implementing_socket`, the previous coding accessed the `sockets` instance variable in two CHECKs without acquiring `mutex`. This is unsafe in general: `mutex` should be acquired before accessing instance variables like `sockets` (unless you know all such concurrent accesses will be read-only etc.). As it happens, the previous coding was not unsafe because all the call-sites of `SocketManager::swap_implementing_socket` acquire `mutex` before invoking it. But because `swap_implementing_socket` also acquires `mutex`, clearly its API contract allows it to be invoked without holding `mutex`, in which case the CHECKs would likely be unsafe. Diffs - 3rdparty/libprocess/src/process.cpp 5e9dcfdc52f3a8223bc43af149b8e1f5dbdf5b0a Diff: https://reviews.apache.org/r/46024/diff/ Testing --- Thanks, Neil Conway
