Re: Review Request 47091: Made http::internal::ConnectionProcess a managed Process.

2016-05-09 Thread Anand Mazumdar

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


Fix it, then Ship it!




LGTM. Just a couple of micro nit-picks.


3rdparty/libprocess/src/http.cpp (line 1160)


Nit: Backticks around `ConnectionProcess`.



3rdparty/libprocess/src/http.cpp (line 1161)


s/ConnectionProcess/process



3rdparty/libprocess/src/http.cpp (lines 1164 - 1167)


hmm.. thinking about it more, we should follow this pattern everywhere. 
More often then not this is what our pimpl pattern looks like:

```cpp
process::terminate(...);
process::wait(..);
delete process; // even better have an `Owned` to clean up the pointer and 
eliminate this line.
```

The last 2 lines are redundant and we can easily get away with libprocess 
doing the GC. This modifies the code to be just a one liner with the added 
benefit of having a `PID` member variable for `dispatch` instead of a pointer:

```cpp
process::terminate(...);
```


- Anand Mazumdar


On May 8, 2016, 12:45 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47091/
> ---
> 
> (Updated May 8, 2016, 12:45 a.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Bugs: MESOS-4658
> https://issues.apache.org/jira/browse/MESOS-4658
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Per the details in MESOS-4658, the `http::Connection` abstraction is
> prone to the deadlock of `process::wait`ing on itself. This occurs
> when the `http::internal::ConnectionProcess` exposes a Future on
> which the caller binds a copy of `http::Connection`. When completing
> the Future, the Future clears its callbacks within the execution
> context of the `http::internal::ConnectionProcess`. If the last copy
> of the `http::Connection` was present within one of the callbacks,
> the `http::internal::ConnectionProcess` will wait on itself, leading
> to a deadlock.
> 
> This surfaces a general pattern of having libprocess manage a
> Process when it cannot be guaranteed that the Process will be
> waited on by another execution context.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/http.cpp 48f91d01555e760b1c4fd2cde684168d65f75e57 
> 
> Diff: https://reviews.apache.org/r/47091/diff/
> 
> 
> Testing
> ---
> 
> Removed the hack put in place within the http::get path to validate the fix.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>



Re: Review Request 47091: Made http::internal::ConnectionProcess a managed Process.

2016-05-07 Thread Mesos ReviewBot

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



Patch looks great!

Reviews applied: [47090, 47091]

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 May 8, 2016, 12:45 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47091/
> ---
> 
> (Updated May 8, 2016, 12:45 a.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Bugs: MESOS-4658
> https://issues.apache.org/jira/browse/MESOS-4658
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Per the details in MESOS-4658, the `http::Connection` abstraction is
> prone to the deadlock of `process::wait`ing on itself. This occurs
> when the `http::internal::ConnectionProcess` exposes a Future on
> which the caller binds a copy of `http::Connection`. When completing
> the Future, the Future clears its callbacks within the execution
> context of the `http::internal::ConnectionProcess`. If the last copy
> of the `http::Connection` was present within one of the callbacks,
> the `http::internal::ConnectionProcess` will wait on itself, leading
> to a deadlock.
> 
> This surfaces a general pattern of having libprocess manage a
> Process when it cannot be guaranteed that the Process will be
> waited on by another execution context.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/http.cpp 48f91d01555e760b1c4fd2cde684168d65f75e57 
> 
> Diff: https://reviews.apache.org/r/47091/diff/
> 
> 
> Testing
> ---
> 
> Removed the hack put in place within the http::get path to validate the fix.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>



Re: Review Request 47091: Made http::internal::ConnectionProcess a managed Process.

2016-05-07 Thread Ben Mahler

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

(Updated May 8, 2016, 12:45 a.m.)


Review request for mesos and Anand Mazumdar.


Changes
---

Removed the accidental dependency.


Bugs: MESOS-4658
https://issues.apache.org/jira/browse/MESOS-4658


Repository: mesos


Description
---

Per the details in MESOS-4658, the `http::Connection` abstraction is
prone to the deadlock of `process::wait`ing on itself. This occurs
when the `http::internal::ConnectionProcess` exposes a Future on
which the caller binds a copy of `http::Connection`. When completing
the Future, the Future clears its callbacks within the execution
context of the `http::internal::ConnectionProcess`. If the last copy
of the `http::Connection` was present within one of the callbacks,
the `http::internal::ConnectionProcess` will wait on itself, leading
to a deadlock.

This surfaces a general pattern of having libprocess manage a
Process when it cannot be guaranteed that the Process will be
waited on by another execution context.


Diffs
-

  3rdparty/libprocess/src/http.cpp 48f91d01555e760b1c4fd2cde684168d65f75e57 

Diff: https://reviews.apache.org/r/47091/diff/


Testing
---

Removed the hack put in place within the http::get path to validate the fix.


Thanks,

Ben Mahler



Re: Review Request 47091: Made http::internal::ConnectionProcess a managed Process.

2016-05-07 Thread Ben Mahler

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

(Updated May 8, 2016, 12:45 a.m.)


Review request for mesos and Anand Mazumdar.


Changes
---

Removed the  include as it is now no longer needed.


Bugs: MESOS-4658
https://issues.apache.org/jira/browse/MESOS-4658


Repository: mesos


Description
---

Per the details in MESOS-4658, the `http::Connection` abstraction is
prone to the deadlock of `process::wait`ing on itself. This occurs
when the `http::internal::ConnectionProcess` exposes a Future on
which the caller binds a copy of `http::Connection`. When completing
the Future, the Future clears its callbacks within the execution
context of the `http::internal::ConnectionProcess`. If the last copy
of the `http::Connection` was present within one of the callbacks,
the `http::internal::ConnectionProcess` will wait on itself, leading
to a deadlock.

This surfaces a general pattern of having libprocess manage a
Process when it cannot be guaranteed that the Process will be
waited on by another execution context.


Diffs (updated)
-

  3rdparty/libprocess/src/http.cpp 48f91d01555e760b1c4fd2cde684168d65f75e57 

Diff: https://reviews.apache.org/r/47091/diff/


Testing
---

Removed the hack put in place within the http::get path to validate the fix.


Thanks,

Ben Mahler



Re: Review Request 47091: Made http::internal::ConnectionProcess a managed Process.

2016-05-07 Thread Ben Mahler

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

(Updated May 8, 2016, 12:41 a.m.)


Review request for mesos and Anand Mazumdar.


Changes
---

Removed the accidental dependency.


Bugs: MESOS-4658
https://issues.apache.org/jira/browse/MESOS-4658


Repository: mesos


Description
---

Per the details in MESOS-4658, the `http::Connection` abstraction is
prone to the deadlock of `process::wait`ing on itself. This occurs
when the `http::internal::ConnectionProcess` exposes a Future on
which the caller binds a copy of `http::Connection`. When completing
the Future, the Future clears its callbacks within the execution
context of the `http::internal::ConnectionProcess`. If the last copy
of the `http::Connection` was present within one of the callbacks,
the `http::internal::ConnectionProcess` will wait on itself, leading
to a deadlock.

This surfaces a general pattern of having libprocess manage a
Process when it cannot be guaranteed that the Process will be
waited on by another execution context.


Diffs
-

  3rdparty/libprocess/src/http.cpp 48f91d01555e760b1c4fd2cde684168d65f75e57 

Diff: https://reviews.apache.org/r/47091/diff/


Testing
---

Removed the hack put in place within the http::get path to validate the fix.


Thanks,

Ben Mahler