Re: Review Request 66863: Reduced likelihood of a stack overflow in libprocess socket send path.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 66863 was successfully built and tested.

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66863

- Mesos Reviewbot Windows


On April 30, 2018, 11:47 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66863/
> ---
> 
> (Updated April 30, 2018, 11:47 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Benno Evers, and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-8594
> https://issues.apache.org/jira/browse/MESOS-8594
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, the socket send path is implemented using an asynchronous
> loop with callbacks. Without using `process::loop`, this pattern is
> prone to a stack overflow in the case that all asynchronous calls
> complete synchronously. This is possible with sockets if the socket
> is always ready for writing. Users have reported the crash in both
> MESOS-8594 and MESOS-8834, so the stack overflow is encountered in
> practice.
> 
> This patch updates the send path to leverage `process::loop`, which
> is supposed to prevent stack overflows in asynchronous loops. However,
> it is still possible for `process::loop` to stack overflow due to
> MESOS-8852. In practice, I expect that even without MESOS-8852 fixed,
> users won't see any stack overflows in the send path.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 21931db5edd7ecf0f4620dba42a5521f48cd47a3 
> 
> 
> Diff: https://reviews.apache.org/r/66863/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 66863: Reduced likelihood of a stack overflow in libprocess socket send path.

[Chun-Hung Hsiao]

> On April 30, 2018, 11:58 p.m., Chun-Hung Hsiao wrote:
> > 3rdparty/libprocess/src/process.cpp
> > Lines 1785 (patched)
> > 
> >
> > `()` can be removed.
> 
> Benjamin Mahler wrote:
> Oh yeah, I often forget about this, I took them out. I guess our rule 
> could either be to always include them or to always omit them if possible?

I suggested to take them out because my impression is that most of the lambdas 
in our codebase omit them if possible.


- Chun-Hung


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


On April 30, 2018, 11:47 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66863/
> ---
> 
> (Updated April 30, 2018, 11:47 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Benno Evers, and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-8594
> https://issues.apache.org/jira/browse/MESOS-8594
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, the socket send path is implemented using an asynchronous
> loop with callbacks. Without using `process::loop`, this pattern is
> prone to a stack overflow in the case that all asynchronous calls
> complete synchronously. This is possible with sockets if the socket
> is always ready for writing. Users have reported the crash in both
> MESOS-8594 and MESOS-8834, so the stack overflow is encountered in
> practice.
> 
> This patch updates the send path to leverage `process::loop`, which
> is supposed to prevent stack overflows in asynchronous loops. However,
> it is still possible for `process::loop` to stack overflow due to
> MESOS-8852. In practice, I expect that even without MESOS-8852 fixed,
> users won't see any stack overflows in the send path.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 21931db5edd7ecf0f4620dba42a5521f48cd47a3 
> 
> 
> Diff: https://reviews.apache.org/r/66863/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 66863: Reduced likelihood of a stack overflow in libprocess socket send path.

[Benjamin Mahler]

> On April 30, 2018, 11:58 p.m., Chun-Hung Hsiao wrote:
> > 3rdparty/libprocess/src/process.cpp
> > Lines 1785 (patched)
> > 
> >
> > `()` can be removed.

Oh yeah, I often forget about this, I took them out. I guess our rule could 
either be to always include them or to always omit them if possible?


- Benjamin


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


On April 30, 2018, 11:47 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66863/
> ---
> 
> (Updated April 30, 2018, 11:47 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Benno Evers, and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-8594
> https://issues.apache.org/jira/browse/MESOS-8594
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, the socket send path is implemented using an asynchronous
> loop with callbacks. Without using `process::loop`, this pattern is
> prone to a stack overflow in the case that all asynchronous calls
> complete synchronously. This is possible with sockets if the socket
> is always ready for writing. Users have reported the crash in both
> MESOS-8594 and MESOS-8834, so the stack overflow is encountered in
> practice.
> 
> This patch updates the send path to leverage `process::loop`, which
> is supposed to prevent stack overflows in asynchronous loops. However,
> it is still possible for `process::loop` to stack overflow due to
> MESOS-8852. In practice, I expect that even without MESOS-8852 fixed,
> users won't see any stack overflows in the send path.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 21931db5edd7ecf0f4620dba42a5521f48cd47a3 
> 
> 
> Diff: https://reviews.apache.org/r/66863/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 66863: Reduced likelihood of a stack overflow in libprocess socket send path.

[Chun-Hung Hsiao]

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


Fix it, then Ship it!





3rdparty/libprocess/src/process.cpp
Lines 1762 (patched)


A space between `()` and `{`. Also `()` can be removed.



3rdparty/libprocess/src/process.cpp
Lines 1785 (patched)


`()` can be removed.



3rdparty/libprocess/src/process.cpp
Lines 1826 (patched)


Do we leave two blank lines between a function and the closing bracelet of 
its namespace?


- Chun-Hung Hsiao


On April 30, 2018, 11:47 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66863/
> ---
> 
> (Updated April 30, 2018, 11:47 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Benno Evers, and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-8594
> https://issues.apache.org/jira/browse/MESOS-8594
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, the socket send path is implemented using an asynchronous
> loop with callbacks. Without using `process::loop`, this pattern is
> prone to a stack overflow in the case that all asynchronous calls
> complete synchronously. This is possible with sockets if the socket
> is always ready for writing. Users have reported the crash in both
> MESOS-8594 and MESOS-8834, so the stack overflow is encountered in
> practice.
> 
> This patch updates the send path to leverage `process::loop`, which
> is supposed to prevent stack overflows in asynchronous loops. However,
> it is still possible for `process::loop` to stack overflow due to
> MESOS-8852. In practice, I expect that even without MESOS-8852 fixed,
> users won't see any stack overflows in the send path.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 21931db5edd7ecf0f4620dba42a5521f48cd47a3 
> 
> 
> Diff: https://reviews.apache.org/r/66863/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 66863: Reduced likelihood of a stack overflow in libprocess socket send path.

[Benjamin Mahler]

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

(Updated April 30, 2018, 11:47 p.m.)


Review request for mesos, Benjamin Hindman, Benno Evers, and Chun-Hung Hsiao.


Changes
---

Updated per chun's suggestion.


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


Repository: mesos


Description
---

Currently, the socket send path is implemented using an asynchronous
loop with callbacks. Without using `process::loop`, this pattern is
prone to a stack overflow in the case that all asynchronous calls
complete synchronously. This is possible with sockets if the socket
is always ready for writing. Users have reported the crash in both
MESOS-8594 and MESOS-8834, so the stack overflow is encountered in
practice.

This patch updates the send path to leverage `process::loop`, which
is supposed to prevent stack overflows in asynchronous loops. However,
it is still possible for `process::loop` to stack overflow due to
MESOS-8852. In practice, I expect that even without MESOS-8852 fixed,
users won't see any stack overflows in the send path.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp 21931db5edd7ecf0f4620dba42a5521f48cd47a3 


Diff: https://reviews.apache.org/r/66863/diff/2/

Changes: https://reviews.apache.org/r/66863/diff/1-2/


Testing
---

make check


Thanks,

Benjamin Mahler

Re: Review Request 66863: Reduced likelihood of a stack overflow in libprocess socket send path.

[Chun-Hung Hsiao]

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


Fix it, then Ship it!





3rdparty/libprocess/src/process.cpp
Lines 1762-1772 (patched)


Alternatively, we could do
```
_send(encoder, socket).then([=] {
  // Loop until this socket has no more encoded messages to send.
  return process::loop(
  None(),
  [=] { return socket_manager->next(socket); }
  [=](Encoder* encoder) -> Future {
if (encoder == nullptr) {
  return Break();
}

return _send(encoder, socket)
  .then([]() -> ControlFlow { return Continue(); });
  })
  });
```
And clean up `encoder` in `_send`:
```
return process::loop(
None(),
[=]() {
  ...
  return send
.then(...)
.recover([=](const Future& f) {
  socket_manager->close(socket);
  delete encoder;
  return f; // Break the loop by propagating the "failure".
});
},
[=](Nothing) -> ControlFlow {
  if (encoder->remaining() == 0) {
delete encoder;
return Break();
  }
  return Continue();
});
```

Please feel free to drop this if you prefer the current version.



3rdparty/libprocess/src/process.cpp
Lines 1779 (patched)


How about `const Future& f`?



3rdparty/libprocess/src/process.cpp
Lines 1823 (patched)


Seems we have code with both `(Nothing)` and `(const Nothing&)`, so I'm not 
raising an issue here.


- Chun-Hung Hsiao


On April 29, 2018, 1:40 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66863/
> ---
> 
> (Updated April 29, 2018, 1:40 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Benno Evers, and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-8594
> https://issues.apache.org/jira/browse/MESOS-8594
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, the socket send path is implemented using an asynchronous
> loop with callbacks. Without using `process::loop`, this pattern is
> prone to a stack overflow in the case that all asynchronous calls
> complete synchronously. This is possible with sockets if the socket
> is always ready for writing. Users have reported the crash in both
> MESOS-8594 and MESOS-8834, so the stack overflow is encountered in
> practice.
> 
> This patch updates the send path to leverage `process::loop`, which
> is supposed to prevent stack overflows in asynchronous loops. However,
> it is still possible for `process::loop` to stack overflow due to
> MESOS-8852. In practice, I expect that even without MESOS-8852 fixed,
> users won't see any stack overflows in the send path.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 21931db5edd7ecf0f4620dba42a5521f48cd47a3 
> 
> 
> Diff: https://reviews.apache.org/r/66863/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 66863: Reduced likelihood of a stack overflow in libprocess socket send path.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66863]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 
MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On April 28, 2018, 9:40 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66863/
> ---
> 
> (Updated April 28, 2018, 9:40 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Benno Evers, and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-8594
> https://issues.apache.org/jira/browse/MESOS-8594
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, the socket send path is implemented using an asynchronous
> loop with callbacks. Without using `process::loop`, this pattern is
> prone to a stack overflow in the case that all asynchronous calls
> complete synchronously. This is possible with sockets if the socket
> is always ready for writing. Users have reported the crash in both
> MESOS-8594 and MESOS-8834, so the stack overflow is encountered in
> practice.
> 
> This patch updates the send path to leverage `process::loop`, which
> is supposed to prevent stack overflows in asynchronous loops. However,
> it is still possible for `process::loop` to stack overflow due to
> MESOS-8852. In practice, I expect that even without MESOS-8852 fixed,
> users won't see any stack overflows in the send path.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 21931db5edd7ecf0f4620dba42a5521f48cd47a3 
> 
> 
> Diff: https://reviews.apache.org/r/66863/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 66863: Reduced likelihood of a stack overflow in libprocess socket send path.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 66863 was successfully built and tested.

Reviews applied: `['66863']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66863

- Mesos Reviewbot Windows


On April 29, 2018, 1:40 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66863/
> ---
> 
> (Updated April 29, 2018, 1:40 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Benno Evers, and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-8594
> https://issues.apache.org/jira/browse/MESOS-8594
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, the socket send path is implemented using an asynchronous
> loop with callbacks. Without using `process::loop`, this pattern is
> prone to a stack overflow in the case that all asynchronous calls
> complete synchronously. This is possible with sockets if the socket
> is always ready for writing. Users have reported the crash in both
> MESOS-8594 and MESOS-8834, so the stack overflow is encountered in
> practice.
> 
> This patch updates the send path to leverage `process::loop`, which
> is supposed to prevent stack overflows in asynchronous loops. However,
> it is still possible for `process::loop` to stack overflow due to
> MESOS-8852. In practice, I expect that even without MESOS-8852 fixed,
> users won't see any stack overflows in the send path.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 21931db5edd7ecf0f4620dba42a5521f48cd47a3 
> 
> 
> Diff: https://reviews.apache.org/r/66863/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>