Re: Review Request 52997: Improve Socket::connect error message.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52997]

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

- Mesos ReviewBot


On Nov. 1, 2016, 12:22 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52997/
> ---
> 
> (Updated Nov. 1, 2016, 12:22 a.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Michael Park.
> 
> 
> Bugs: MESOS-6412
> https://issues.apache.org/jira/browse/MESOS-6412
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Rather than returning a fixed generic error when connect(2) fails,
> report the actual socket error as well as the address we were trying
> to connect to.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/poll_socket.cpp 
> d9ab3fbbf385c39c0e43ee92ff06609ae8d47e2d 
> 
> Diff: https://reviews.apache.org/r/52997/diff/
> 
> 
> Testing
> ---
> 
> `make check` on a host whose hostname doesn't match its IP address.
> 
> ```
> [jpeach@jpeach libprocess]$ ./libprocess-tests 
> --gtest_filter=HTTPTest.Endpoints
> Note: Google Test filter = HTTPTest.Endpoints
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from HTTPTest
> [ RUN  ] HTTPTest.Endpoints
> ../../../3rdparty/libprocess/src/tests/http_tests.cpp:141: Failure
> (socket.connect(http.process->self().address)).failure(): Failed to connect 
> to 17.203.52.49:39241: Connection refused
> [  FAILED  ] HTTPTest.Endpoints (4 ms)
> [--] 1 test from HTTPTest (4 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (5 ms total)
> [  PASSED  ] 0 tests.
> [  FAILED  ] 1 test, listed below:
> [  FAILED  ] HTTPTest.Endpoints
> 
>  1 FAILED TEST
> ```
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 52997: Improve Socket::connect error message.

[James Peach]

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

(Updated Nov. 1, 2016, 12:22 a.m.)


Review request for mesos, Joris Van Remoortere and Michael Park.


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


Repository: mesos


Description (updated)
---

Rather than returning a fixed generic error when connect(2) fails,
report the actual socket error as well as the address we were trying
to connect to.


Diffs (updated)
-

  3rdparty/libprocess/src/poll_socket.cpp 
d9ab3fbbf385c39c0e43ee92ff06609ae8d47e2d 

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


Testing
---

`make check` on a host whose hostname doesn't match its IP address.

```
[jpeach@jpeach libprocess]$ ./libprocess-tests --gtest_filter=HTTPTest.Endpoints
Note: Google Test filter = HTTPTest.Endpoints
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from HTTPTest
[ RUN  ] HTTPTest.Endpoints
../../../3rdparty/libprocess/src/tests/http_tests.cpp:141: Failure
(socket.connect(http.process->self().address)).failure(): Failed to connect to 
17.203.52.49:39241: Connection refused
[  FAILED  ] HTTPTest.Endpoints (4 ms)
[--] 1 test from HTTPTest (4 ms total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (5 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] HTTPTest.Endpoints

 1 FAILED TEST
```


Thanks,

James Peach

Re: Review Request 52997: Improve Socket::connect error message.

[Michael Park]

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


Fix it, then Ship it!





3rdparty/libprocess/src/poll_socket.cpp (lines 133 - 138)


Would you mind leaving a comment here mentioning that we're looking at 
`opt` because of the use of `SO_ERROR`, and the fact that this is a temporary 
solution until we fix the `ErrnoError` design?


- Michael Park


On Oct. 20, 2016, 9:07 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52997/
> ---
> 
> (Updated Oct. 20, 2016, 9:07 a.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Michael Park.
> 
> 
> Bugs: MESOS-6412
> https://issues.apache.org/jira/browse/MESOS-6412
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Rather than returning a fixed generic error when connect(2) fails,
> report the actual socket error as well as the address we were trying to
> connect to.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/poll_socket.cpp 
> d9ab3fbbf385c39c0e43ee92ff06609ae8d47e2d 
> 
> Diff: https://reviews.apache.org/r/52997/diff/
> 
> 
> Testing
> ---
> 
> `make check` on a host whose hostname doesn't match its IP address.
> 
> ```
> [jpeach@jpeach libprocess]$ ./libprocess-tests 
> --gtest_filter=HTTPTest.Endpoints
> Note: Google Test filter = HTTPTest.Endpoints
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from HTTPTest
> [ RUN  ] HTTPTest.Endpoints
> ../../../3rdparty/libprocess/src/tests/http_tests.cpp:141: Failure
> (socket.connect(http.process->self().address)).failure(): Failed to connect 
> to 17.203.52.49:39241: Connection refused
> [  FAILED  ] HTTPTest.Endpoints (4 ms)
> [--] 1 test from HTTPTest (4 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (5 ms total)
> [  PASSED  ] 0 tests.
> [  FAILED  ] 1 test, listed below:
> [  FAILED  ] HTTPTest.Endpoints
> 
>  1 FAILED TEST
> ```
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 52997: Improve Socket::connect error message.

[Michael Park]

> On Oct. 18, 2016, 10:32 p.m., Michael Park wrote:
> > 3rdparty/libprocess/src/poll_socket.cpp, lines 133-141
> > 
> >
> > Do we need to set these here...? If I understand correctly they already 
> > hold the correct values, and we would be overwriting it with an incorrect 
> > one.
> > 
> > Assuming we want `SocketError` on line 140 to build an error message 
> > based on the corresponding error code, `getsockopt` automatically sets the 
> > `errno` and `WSALastError` correctly.
> > 
> > Linux:
> > 
> > > On success, zero is returned for the standard options.  On error, -1 
> > is returned, and errno is set appropriately.
> > 
> > Windows:
> > 
> > > If no error occurs, getsockopt returns zero. Otherwise, a value of 
> > SOCKET_ERROR is returned, and a specific error code can be retrieved by 
> > calling WSAGetLastError.
> 
> James Peach wrote:
> ``getsockopt`` is returning the ``errno`` from a previous ``connect(2)`` 
> call into ``opt``. ``errno`` won't be set in this case unless the 
> ``getsockopt`` succeeds. The actual ``errno`` value is in ``opt`` and we need 
> to set the thread-local socket API error so that ``SocketError`` can have it.

I realized that I missed the use of `SO_ERROR` here :(


- Michael


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


On Oct. 20, 2016, 9:07 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52997/
> ---
> 
> (Updated Oct. 20, 2016, 9:07 a.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Michael Park.
> 
> 
> Bugs: MESOS-6412
> https://issues.apache.org/jira/browse/MESOS-6412
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Rather than returning a fixed generic error when connect(2) fails,
> report the actual socket error as well as the address we were trying to
> connect to.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/poll_socket.cpp 
> d9ab3fbbf385c39c0e43ee92ff06609ae8d47e2d 
> 
> Diff: https://reviews.apache.org/r/52997/diff/
> 
> 
> Testing
> ---
> 
> `make check` on a host whose hostname doesn't match its IP address.
> 
> ```
> [jpeach@jpeach libprocess]$ ./libprocess-tests 
> --gtest_filter=HTTPTest.Endpoints
> Note: Google Test filter = HTTPTest.Endpoints
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from HTTPTest
> [ RUN  ] HTTPTest.Endpoints
> ../../../3rdparty/libprocess/src/tests/http_tests.cpp:141: Failure
> (socket.connect(http.process->self().address)).failure(): Failed to connect 
> to 17.203.52.49:39241: Connection refused
> [  FAILED  ] HTTPTest.Endpoints (4 ms)
> [--] 1 test from HTTPTest (4 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (5 ms total)
> [  PASSED  ] 0 tests.
> [  FAILED  ] 1 test, listed below:
> [  FAILED  ] HTTPTest.Endpoints
> 
>  1 FAILED TEST
> ```
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 52997: Improve Socket::connect error message.

[James Peach]

> On Oct. 19, 2016, 5:32 a.m., Michael Park wrote:
> > 3rdparty/libprocess/src/poll_socket.cpp, lines 133-141
> > 
> >
> > Do we need to set these here...? If I understand correctly they already 
> > hold the correct values, and we would be overwriting it with an incorrect 
> > one.
> > 
> > Assuming we want `SocketError` on line 140 to build an error message 
> > based on the corresponding error code, `getsockopt` automatically sets the 
> > `errno` and `WSALastError` correctly.
> > 
> > Linux:
> > 
> > > On success, zero is returned for the standard options.  On error, -1 
> > is returned, and errno is set appropriately.
> > 
> > Windows:
> > 
> > > If no error occurs, getsockopt returns zero. Otherwise, a value of 
> > SOCKET_ERROR is returned, and a specific error code can be retrieved by 
> > calling WSAGetLastError.

``getsockopt`` is returning the ``errno`` from a previous ``connect(2)`` call 
into ``opt``. ``errno`` won't be set in this case unless the ``getsockopt`` 
succeeds. The actual ``errno`` value is in ``opt`` and we need to set the 
thread-local socket API error so that ``SocketError`` can have it.


- James


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


On Oct. 18, 2016, 10:43 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52997/
> ---
> 
> (Updated Oct. 18, 2016, 10:43 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-6412
> https://issues.apache.org/jira/browse/MESOS-6412
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Rather than returning a fixed generic error when connect(2) fails,
> report the actual socket error as well as the address we were trying to
> connect to.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/poll_socket.cpp 
> d9ab3fbbf385c39c0e43ee92ff06609ae8d47e2d 
> 
> Diff: https://reviews.apache.org/r/52997/diff/
> 
> 
> Testing
> ---
> 
> `make check` on a host whose hostname doesn't match its IP address.
> 
> ```
> [jpeach@jpeach libprocess]$ ./libprocess-tests 
> --gtest_filter=HTTPTest.Endpoints
> Note: Google Test filter = HTTPTest.Endpoints
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from HTTPTest
> [ RUN  ] HTTPTest.Endpoints
> ../../../3rdparty/libprocess/src/tests/http_tests.cpp:141: Failure
> (socket.connect(http.process->self().address)).failure(): Failed to connect 
> to 17.203.52.49:39241: Connection refused
> [  FAILED  ] HTTPTest.Endpoints (4 ms)
> [--] 1 test from HTTPTest (4 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (5 ms total)
> [  PASSED  ] 0 tests.
> [  FAILED  ] 1 test, listed below:
> [  FAILED  ] HTTPTest.Endpoints
> 
>  1 FAILED TEST
> ```
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 52997: Improve Socket::connect error message.

[Michael Park]

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




3rdparty/libprocess/src/poll_socket.cpp (lines 133 - 141)


Do we need to set these here...? If I understand correctly they already 
hold the correct values, and we would be overwriting it with an incorrect one.

Assuming we want `SocketError` on line 140 to build an error message based 
on the corresponding error code, `getsockopt` automatically sets the `errno` 
and `WSALastError` correctly.

Linux:

> On success, zero is returned for the standard options.  On error, -1 is 
returned, and errno is set appropriately.

Windows:

> If no error occurs, getsockopt returns zero. Otherwise, a value of 
SOCKET_ERROR is returned, and a specific error code can be retrieved by calling 
WSAGetLastError.


- Michael Park


On Oct. 18, 2016, 10:43 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52997/
> ---
> 
> (Updated Oct. 18, 2016, 10:43 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-6412
> https://issues.apache.org/jira/browse/MESOS-6412
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Rather than returning a fixed generic error when connect(2) fails,
> report the actual socket error as well as the address we were trying to
> connect to.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/poll_socket.cpp 
> d9ab3fbbf385c39c0e43ee92ff06609ae8d47e2d 
> 
> Diff: https://reviews.apache.org/r/52997/diff/
> 
> 
> Testing
> ---
> 
> `make check` on a host whose hostname doesn't match its IP address.
> 
> ```
> [jpeach@jpeach libprocess]$ ./libprocess-tests 
> --gtest_filter=HTTPTest.Endpoints
> Note: Google Test filter = HTTPTest.Endpoints
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from HTTPTest
> [ RUN  ] HTTPTest.Endpoints
> ../../../3rdparty/libprocess/src/tests/http_tests.cpp:141: Failure
> (socket.connect(http.process->self().address)).failure(): Failed to connect 
> to 17.203.52.49:39241: Connection refused
> [  FAILED  ] HTTPTest.Endpoints (4 ms)
> [--] 1 test from HTTPTest (4 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (5 ms total)
> [  PASSED  ] 0 tests.
> [  FAILED  ] 1 test, listed below:
> [  FAILED  ] HTTPTest.Endpoints
> 
>  1 FAILED TEST
> ```
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 52997: Improve Socket::connect error message.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52997]

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

- Mesos ReviewBot


On Oct. 18, 2016, 10:43 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52997/
> ---
> 
> (Updated Oct. 18, 2016, 10:43 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-6412
> https://issues.apache.org/jira/browse/MESOS-6412
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Rather than returning a fixed generic error when connect(2) fails,
> report the actual socket error as well as the address we were trying to
> connect to.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/poll_socket.cpp 
> d9ab3fbbf385c39c0e43ee92ff06609ae8d47e2d 
> 
> Diff: https://reviews.apache.org/r/52997/diff/
> 
> 
> Testing
> ---
> 
> `make check` on a host whose hostname doesn't match its IP address.
> 
> ```
> [jpeach@jpeach libprocess]$ ./libprocess-tests 
> --gtest_filter=HTTPTest.Endpoints
> Note: Google Test filter = HTTPTest.Endpoints
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from HTTPTest
> [ RUN  ] HTTPTest.Endpoints
> ../../../3rdparty/libprocess/src/tests/http_tests.cpp:141: Failure
> (socket.connect(http.process->self().address)).failure(): Failed to connect 
> to 17.203.52.49:39241: Connection refused
> [  FAILED  ] HTTPTest.Endpoints (4 ms)
> [--] 1 test from HTTPTest (4 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (5 ms total)
> [  PASSED  ] 0 tests.
> [  FAILED  ] 1 test, listed below:
> [  FAILED  ] HTTPTest.Endpoints
> 
>  1 FAILED TEST
> ```
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 52997: Improve Socket::connect error message.

[James Peach]

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

(Updated Oct. 18, 2016, 10:43 p.m.)


Review request for mesos and Michael Park.


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


Repository: mesos


Description
---

Rather than returning a fixed generic error when connect(2) fails,
report the actual socket error as well as the address we were trying to
connect to.


Diffs (updated)
-

  3rdparty/libprocess/src/poll_socket.cpp 
d9ab3fbbf385c39c0e43ee92ff06609ae8d47e2d 

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


Testing
---

`make check` on a host whose hostname doesn't match its IP address.

```
[jpeach@jpeach libprocess]$ ./libprocess-tests --gtest_filter=HTTPTest.Endpoints
Note: Google Test filter = HTTPTest.Endpoints
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from HTTPTest
[ RUN  ] HTTPTest.Endpoints
../../../3rdparty/libprocess/src/tests/http_tests.cpp:141: Failure
(socket.connect(http.process->self().address)).failure(): Failed to connect to 
17.203.52.49:39241: Connection refused
[  FAILED  ] HTTPTest.Endpoints (4 ms)
[--] 1 test from HTTPTest (4 ms total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (5 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] HTTPTest.Endpoints

 1 FAILED TEST
```


Thanks,

James Peach

Re: Review Request 52997: Improve Socket::connect error message.

[James Peach]

> On Oct. 18, 2016, 9:41 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/src/poll_socket.cpp, line 114
> > 
> >
> > Missing reference.
> 
> James Peach wrote:
> I wanted a copy (at least the lambda needs to take a copy), since 
> ``connect`` takes a reference to the address.
> 
> Joris Van Remoortere wrote:
> It should be a copy by default unless you `cref` it right?
> I think you can make the parameter a `const Address& to`.
> Mpark do you have an opinion?

Ok, `std::decay` says it removes the reference, so there's an implicit copy 
into the lambda here.


On Oct. 18, 2016, 9:41 p.m., James Peach wrote:
> > Can you also follow up with a review to change the logging level in 
> > libprocess for failing to close the socket if we fail to connect?
> > Maybe it should be `VLOG(1)`? Thoughts?
> 
> James Peach wrote:
> I actually removed the `VLOG` thinking that it would be better for the 
> caller to log the error since it can provide context.
> 
> Joris Van Remoortere wrote:
> I meant these 2:
> 
> https://github.com/apache/mesos/blob/fd1b7bdfb2aa8a6f3dffab9ce8114c1a3c183e23/3rdparty/libprocess/src/process.cpp#L2071
> 
> https://github.com/apache/mesos/blob/fd1b7bdfb2aa8a6f3dffab9ce8114c1a3c183e23/3rdparty/libprocess/src/process.cpp#L2154

Oh I see. I haven't really looked at that code, though I recall seeing spurious 
errors from it in the past. In most cases I see through that code path a socket 
that is not connected would not be added to the socket manager. Maybe reconnect 
has a way though.


- James


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


On Oct. 18, 2016, 9:37 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52997/
> ---
> 
> (Updated Oct. 18, 2016, 9:37 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-6412
> https://issues.apache.org/jira/browse/MESOS-6412
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Rather than returning a fixed generic error when connect(2) fails,
> report the actual socket error as well as the address we were trying to
> connect to.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/poll_socket.cpp 
> d9ab3fbbf385c39c0e43ee92ff06609ae8d47e2d 
> 
> Diff: https://reviews.apache.org/r/52997/diff/
> 
> 
> Testing
> ---
> 
> `make check` on a host whose hostname doesn't match its IP address.
> 
> ```
> [jpeach@jpeach libprocess]$ ./libprocess-tests 
> --gtest_filter=HTTPTest.Endpoints
> Note: Google Test filter = HTTPTest.Endpoints
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from HTTPTest
> [ RUN  ] HTTPTest.Endpoints
> ../../../3rdparty/libprocess/src/tests/http_tests.cpp:141: Failure
> (socket.connect(http.process->self().address)).failure(): Failed to connect 
> to 17.203.52.49:39241: Connection refused
> [  FAILED  ] HTTPTest.Endpoints (4 ms)
> [--] 1 test from HTTPTest (4 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (5 ms total)
> [  PASSED  ] 0 tests.
> [  FAILED  ] 1 test, listed below:
> [  FAILED  ] HTTPTest.Endpoints
> 
>  1 FAILED TEST
> ```
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 52997: Improve Socket::connect error message.

[Joris Van Remoortere]

On Oct. 18, 2016, 9:41 p.m., James Peach wrote:
> > Can you also follow up with a review to change the logging level in 
> > libprocess for failing to close the socket if we fail to connect?
> > Maybe it should be `VLOG(1)`? Thoughts?
> 
> James Peach wrote:
> I actually removed the `VLOG` thinking that it would be better for the 
> caller to log the error since it can provide context.

I meant these 2:
https://github.com/apache/mesos/blob/fd1b7bdfb2aa8a6f3dffab9ce8114c1a3c183e23/3rdparty/libprocess/src/process.cpp#L2071
https://github.com/apache/mesos/blob/fd1b7bdfb2aa8a6f3dffab9ce8114c1a3c183e23/3rdparty/libprocess/src/process.cpp#L2154


- Joris


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


On Oct. 18, 2016, 9:37 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52997/
> ---
> 
> (Updated Oct. 18, 2016, 9:37 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-6412
> https://issues.apache.org/jira/browse/MESOS-6412
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Rather than returning a fixed generic error when connect(2) fails,
> report the actual socket error as well as the address we were trying to
> connect to.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/poll_socket.cpp 
> d9ab3fbbf385c39c0e43ee92ff06609ae8d47e2d 
> 
> Diff: https://reviews.apache.org/r/52997/diff/
> 
> 
> Testing
> ---
> 
> `make check` on a host whose hostname doesn't match its IP address.
> 
> ```
> [jpeach@jpeach libprocess]$ ./libprocess-tests 
> --gtest_filter=HTTPTest.Endpoints
> Note: Google Test filter = HTTPTest.Endpoints
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from HTTPTest
> [ RUN  ] HTTPTest.Endpoints
> ../../../3rdparty/libprocess/src/tests/http_tests.cpp:141: Failure
> (socket.connect(http.process->self().address)).failure(): Failed to connect 
> to 17.203.52.49:39241: Connection refused
> [  FAILED  ] HTTPTest.Endpoints (4 ms)
> [--] 1 test from HTTPTest (4 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (5 ms total)
> [  PASSED  ] 0 tests.
> [  FAILED  ] 1 test, listed below:
> [  FAILED  ] HTTPTest.Endpoints
> 
>  1 FAILED TEST
> ```
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 52997: Improve Socket::connect error message.

[Joris Van Remoortere]

> On Oct. 18, 2016, 9:41 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/src/poll_socket.cpp, line 114
> > 
> >
> > Missing reference.
> 
> James Peach wrote:
> I wanted a copy (at least the lambda needs to take a copy), since 
> ``connect`` takes a reference to the address.

It should be a copy by default unless you `cref` it right?
I think you can make the parameter a `const Address& to`.
Mpark do you have an opinion?


- Joris


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


On Oct. 18, 2016, 9:37 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52997/
> ---
> 
> (Updated Oct. 18, 2016, 9:37 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-6412
> https://issues.apache.org/jira/browse/MESOS-6412
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Rather than returning a fixed generic error when connect(2) fails,
> report the actual socket error as well as the address we were trying to
> connect to.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/poll_socket.cpp 
> d9ab3fbbf385c39c0e43ee92ff06609ae8d47e2d 
> 
> Diff: https://reviews.apache.org/r/52997/diff/
> 
> 
> Testing
> ---
> 
> `make check` on a host whose hostname doesn't match its IP address.
> 
> ```
> [jpeach@jpeach libprocess]$ ./libprocess-tests 
> --gtest_filter=HTTPTest.Endpoints
> Note: Google Test filter = HTTPTest.Endpoints
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from HTTPTest
> [ RUN  ] HTTPTest.Endpoints
> ../../../3rdparty/libprocess/src/tests/http_tests.cpp:141: Failure
> (socket.connect(http.process->self().address)).failure(): Failed to connect 
> to 17.203.52.49:39241: Connection refused
> [  FAILED  ] HTTPTest.Endpoints (4 ms)
> [--] 1 test from HTTPTest (4 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (5 ms total)
> [  PASSED  ] 0 tests.
> [  FAILED  ] 1 test, listed below:
> [  FAILED  ] HTTPTest.Endpoints
> 
>  1 FAILED TEST
> ```
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 52997: Improve Socket::connect error message.

[James Peach]

> On Oct. 18, 2016, 9:41 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/src/poll_socket.cpp, line 114
> > 
> >
> > Missing reference.

I wanted a copy (at least the lambda needs to take a copy), since ``connect`` 
takes a reference to the address.


On Oct. 18, 2016, 9:41 p.m., James Peach wrote:
> > Can you also follow up with a review to change the logging level in 
> > libprocess for failing to close the socket if we fail to connect?
> > Maybe it should be `VLOG(1)`? Thoughts?

I actually removed the `VLOG` thinking that it would be better for the caller 
to log the error since it can provide context.


- James


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


On Oct. 18, 2016, 9:37 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52997/
> ---
> 
> (Updated Oct. 18, 2016, 9:37 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-6412
> https://issues.apache.org/jira/browse/MESOS-6412
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Rather than returning a fixed generic error when connect(2) fails,
> report the actual socket error as well as the address we were trying to
> connect to.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/poll_socket.cpp 
> d9ab3fbbf385c39c0e43ee92ff06609ae8d47e2d 
> 
> Diff: https://reviews.apache.org/r/52997/diff/
> 
> 
> Testing
> ---
> 
> `make check` on a host whose hostname doesn't match its IP address.
> 
> ```
> [jpeach@jpeach libprocess]$ ./libprocess-tests 
> --gtest_filter=HTTPTest.Endpoints
> Note: Google Test filter = HTTPTest.Endpoints
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from HTTPTest
> [ RUN  ] HTTPTest.Endpoints
> ../../../3rdparty/libprocess/src/tests/http_tests.cpp:141: Failure
> (socket.connect(http.process->self().address)).failure(): Failed to connect 
> to 17.203.52.49:39241: Connection refused
> [  FAILED  ] HTTPTest.Endpoints (4 ms)
> [--] 1 test from HTTPTest (4 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (5 ms total)
> [  PASSED  ] 0 tests.
> [  FAILED  ] 1 test, listed below:
> [  FAILED  ] HTTPTest.Endpoints
> 
>  1 FAILED TEST
> ```
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 52997: Improve Socket::connect error message.

[Joris Van Remoortere]

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




3rdparty/libprocess/src/poll_socket.cpp (line 114)


Missing reference.


Can you also follow up with a review to change the logging level in libprocess 
for failing to close the socket if we fail to connect?
Maybe it should be `VLOG(1)`? Thoughts?

- Joris Van Remoortere


On Oct. 18, 2016, 9:37 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52997/
> ---
> 
> (Updated Oct. 18, 2016, 9:37 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-6412
> https://issues.apache.org/jira/browse/MESOS-6412
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Rather than returning a fixed generic error when connect(2) fails,
> report the actual socket error as well as the address we were trying to
> connect to.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/poll_socket.cpp 
> d9ab3fbbf385c39c0e43ee92ff06609ae8d47e2d 
> 
> Diff: https://reviews.apache.org/r/52997/diff/
> 
> 
> Testing
> ---
> 
> `make check` on a host whose hostname doesn't match its IP address.
> 
> ```
> [jpeach@jpeach libprocess]$ ./libprocess-tests 
> --gtest_filter=HTTPTest.Endpoints
> Note: Google Test filter = HTTPTest.Endpoints
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from HTTPTest
> [ RUN  ] HTTPTest.Endpoints
> ../../../3rdparty/libprocess/src/tests/http_tests.cpp:141: Failure
> (socket.connect(http.process->self().address)).failure(): Failed to connect 
> to 17.203.52.49:39241: Connection refused
> [  FAILED  ] HTTPTest.Endpoints (4 ms)
> [--] 1 test from HTTPTest (4 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (5 ms total)
> [  PASSED  ] 0 tests.
> [  FAILED  ] 1 test, listed below:
> [  FAILED  ] HTTPTest.Endpoints
> 
>  1 FAILED TEST
> ```
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 52997: Improve Socket::connect error message.

[James Peach]

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

(Updated Oct. 18, 2016, 9:37 p.m.)


Review request for mesos and Michael Park.


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


Repository: mesos


Description
---

Rather than returning a fixed generic error when connect(2) fails,
report the actual socket error as well as the address we were trying to
connect to.


Diffs
-

  3rdparty/libprocess/src/poll_socket.cpp 
d9ab3fbbf385c39c0e43ee92ff06609ae8d47e2d 

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


Testing (updated)
---

`make check` on a host whose hostname doesn't match its IP address.

```
[jpeach@jpeach libprocess]$ ./libprocess-tests --gtest_filter=HTTPTest.Endpoints
Note: Google Test filter = HTTPTest.Endpoints
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from HTTPTest
[ RUN  ] HTTPTest.Endpoints
../../../3rdparty/libprocess/src/tests/http_tests.cpp:141: Failure
(socket.connect(http.process->self().address)).failure(): Failed to connect to 
17.203.52.49:39241: Connection refused
[  FAILED  ] HTTPTest.Endpoints (4 ms)
[--] 1 test from HTTPTest (4 ms total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (5 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] HTTPTest.Endpoints

 1 FAILED TEST
```


Thanks,

James Peach