You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Michael Park <mp...@apache.org> on 2017/01/10 02:50:18 UTC
Re: Review Request 54602: Replaced `int` with `int_fd` in libprocess.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54602/
-----------------------------------------------------------
(Updated Jan. 9, 2017, 6:50 p.m.)
Review request for mesos, Daniel Pravat and Joris Van Remoortere.
Changes
-------
Rebased.
Repository: mesos
Description
-------
Replaced `int` with `int_fd` in libprocess.
Diffs (updated)
-----
3rdparty/libprocess/include/process/io.hpp f5489dc66adb136d9f53f98ac64c3fbe7831a1c2
3rdparty/libprocess/include/process/network.hpp 8234765e23bb3d434da0b0f818fd42569d554ab8
3rdparty/libprocess/include/process/posix/subprocess.hpp a1054e788ef6a322901c262380aceab8192235ac
3rdparty/libprocess/include/process/socket.hpp 87966155aa21328db51796b2ae0a883054c00457
3rdparty/libprocess/include/process/subprocess_base.hpp 74a4bef0706334b4d3c1040a08a8921fbc300344
3rdparty/libprocess/include/process/windows/subprocess.hpp 3bc7f1992d9c38dac2ec23d5bc57415f37d0318a
3rdparty/libprocess/src/encoder.hpp 1447d6f93c15b9f3d134507ecb0bda020a218a49
3rdparty/libprocess/src/http.cpp b5c38ce89b73788f7446e6c44fd99da6478b064d
3rdparty/libprocess/src/io.cpp 8aa3576a11ed5e998492b4020cb37a45ec708093
3rdparty/libprocess/src/libev_poll.cpp da2a78bd8aa4ed8c37bd2ca16d148b73112aa0e7
3rdparty/libprocess/src/libevent_poll.cpp 0803ba33622c86df38b3efd4f1e3197edf93a0af
3rdparty/libprocess/src/libevent_ssl_socket.hpp 57eaf4f607d0628db466cc1a139772eeeaa51136
3rdparty/libprocess/src/libevent_ssl_socket.cpp dddd0e292a8b0d470f4e199db08f09a0c863d73c
3rdparty/libprocess/src/poll_socket.hpp 89789e6bb91d79e4de1c4f4be3882df851845930
3rdparty/libprocess/src/poll_socket.cpp 93ca37f105527fb225574107480114ee5ac74c76
3rdparty/libprocess/src/process.cpp f475fe78f801924f70f51fdc4ab190c2dbecd656
3rdparty/libprocess/src/socket.cpp b819503095261c77f42d6f20d1a4b2b6170fb4e1
3rdparty/libprocess/src/subprocess.cpp ad19b0896b4a2e9c60f573cc854c10c69e909e86
3rdparty/libprocess/src/subprocess_posix.cpp 19271414f145d23f50ac07570c48782819f382b4
3rdparty/libprocess/src/subprocess_windows.cpp 20cad52d4a4d7fc51487e150a849972eb19ed08e
3rdparty/libprocess/src/tests/io_tests.cpp 466e343e6d775fe09debce119eae4fc4849b7006
3rdparty/libprocess/src/tests/ssl_tests.cpp a32d20e47f67d88bbe5928e0ddc129745c5f42e0
3rdparty/libprocess/src/tests/subprocess_tests.cpp 59c17692012ddfb540ecdd48560c73c42a15f061
Diff: https://reviews.apache.org/r/54602/diff/
Testing
-------
Thanks,
Michael Park
Re: Review Request 54602: Replaced `int` with `int_fd` in libprocess.
Posted by Michael Park <mp...@apache.org>.
> On Feb. 2, 2017, 11:46 a.m., Alex Clemmer wrote:
> > 3rdparty/libprocess/src/io.cpp, line 222
> > <https://reviews.apache.org/r/54602/diff/5/?file=1600504#file1600504line222>
> >
> > This comparison is definitely a bug. On Windows, when this `fd` is a pipe `HANDLE`, this comparison fails even for valid values of `fd`.
> >
> > This can, among other things, cause the Windows Executor to hang indefinitely in some scenarios. For example, when we call `docker inspect`, we will probably end up writing more data to the pipe than the pipe has buffer, so the Executor will block until we `io::read` it. But since this comparison fails, we will never complete the read. Hence, the Executor hangs indefinitely.
> >
> > This comparison can be made to work on Windows by making it properly check whether the `HANDLE` (using, _e.g._, `fd == os::WindowsFd(INVALID_HANDLE_VALUE)`) is invalid, but this obviously won't work on POSIX. In general, I suspect these comparisons are fraught, so I think we should consider removing them and having utility functions such as `fd.valid()` or something.
We explicitly support these comparisons, and they're handled by the complex logic in `operator<` for `WindowsFD`.
There was a bug that you were running into I believe, which I have addressed. Thanks!
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54602/#review164008
-----------------------------------------------------------
On Feb. 4, 2017, 5:39 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54602/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2017, 5:39 p.m.)
>
>
> Review request for mesos, Daniel Pravat and Joris Van Remoortere.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Replaced `int` with `int_fd` in libprocess.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/io.hpp b342333bc7f2ae12b5b6a92fa21896c8f42353cb
> 3rdparty/libprocess/include/process/network.hpp 8234765e23bb3d434da0b0f818fd42569d554ab8
> 3rdparty/libprocess/include/process/posix/subprocess.hpp a1054e788ef6a322901c262380aceab8192235ac
> 3rdparty/libprocess/include/process/socket.hpp 5e958addc6012d85d1dd9025d27b258265807a9e
> 3rdparty/libprocess/include/process/subprocess_base.hpp 1c9f2e33a80e457857985d9b5663c8cb2a089505
> 3rdparty/libprocess/include/process/windows/subprocess.hpp f7beba16d6ed56be45bf5f6221e89a5b0cbcf1e7
> 3rdparty/libprocess/src/encoder.hpp 1447d6f93c15b9f3d134507ecb0bda020a218a49
> 3rdparty/libprocess/src/http.cpp f12a8a5d71f09c7413dae3e6192706ae63972923
> 3rdparty/libprocess/src/io.cpp e31f176a4ef110e9c7d0e5efd88c610e30718d2f
> 3rdparty/libprocess/src/libev_poll.cpp da2a78bd8aa4ed8c37bd2ca16d148b73112aa0e7
> 3rdparty/libprocess/src/libevent_poll.cpp 0803ba33622c86df38b3efd4f1e3197edf93a0af
> 3rdparty/libprocess/src/libevent_ssl_socket.hpp 97ce339c8183b4a4ffc4e37f052c9d2adb79654d
> 3rdparty/libprocess/src/libevent_ssl_socket.cpp fec131f912174e2f9c1b1060a45158d6a61c43c5
> 3rdparty/libprocess/src/poll_socket.hpp 89789e6bb91d79e4de1c4f4be3882df851845930
> 3rdparty/libprocess/src/poll_socket.cpp 93ca37f105527fb225574107480114ee5ac74c76
> 3rdparty/libprocess/src/process.cpp d1331238f6c9df47660bae8cdaa5e8c8add70212
> 3rdparty/libprocess/src/socket.cpp b819503095261c77f42d6f20d1a4b2b6170fb4e1
> 3rdparty/libprocess/src/subprocess.cpp b89dc7ebb86604fad7ca14a9750ef3faff7efbed
> 3rdparty/libprocess/src/subprocess_posix.cpp 19271414f145d23f50ac07570c48782819f382b4
> 3rdparty/libprocess/src/subprocess_windows.cpp 20cad52d4a4d7fc51487e150a849972eb19ed08e
> 3rdparty/libprocess/src/tests/io_tests.cpp 5c889e97cb1402a98b27cad2f3dbb4047a995506
> 3rdparty/libprocess/src/tests/ssl_tests.cpp a32d20e47f67d88bbe5928e0ddc129745c5f42e0
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp 59c17692012ddfb540ecdd48560c73c42a15f061
>
> Diff: https://reviews.apache.org/r/54602/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 54602: Replaced `int` with `int_fd` in libprocess.
Posted by Alex Clemmer <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54602/#review164008
-----------------------------------------------------------
3rdparty/libprocess/src/io.cpp (line 222)
<https://reviews.apache.org/r/54602/#comment235523>
This comparison is definitely a bug. On Windows, when this `fd` is a pipe `HANDLE`, this comparison fails even for valid values of `fd`.
This can, among other things, cause the Windows Executor to hang indefinitely in some scenarios. For example, when we call `docker inspect`, we will probably end up writing more data to the pipe than the pipe has buffer, so the Executor will block until we `io::read` it. But since this comparison fails, we will never complete the read. Hence, the Executor hangs indefinitely.
This comparison can be made to work on Windows by making it properly check whether the `HANDLE` (using, _e.g._, `fd == os::WindowsFd(INVALID_HANDLE_VALUE)`) is invalid, but this obviously won't work on POSIX. In general, I suspect these comparisons are fraught, so I think we should consider removing them and having utility functions such as `fd.valid()` or something.
3rdparty/libprocess/src/io.cpp (line 283)
<https://reviews.apache.org/r/54602/#comment235524>
This also seems to be not a correct comparison? See the comment in `io::read`.
3rdparty/libprocess/src/process.cpp (line 1457)
<https://reviews.apache.org/r/54602/#comment235525>
This also seems to be not a correct comparison? See the comment in `io::read`.
- Alex Clemmer
On Jan. 10, 2017, 2:50 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54602/
> -----------------------------------------------------------
>
> (Updated Jan. 10, 2017, 2:50 a.m.)
>
>
> Review request for mesos, Daniel Pravat and Joris Van Remoortere.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Replaced `int` with `int_fd` in libprocess.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/io.hpp f5489dc66adb136d9f53f98ac64c3fbe7831a1c2
> 3rdparty/libprocess/include/process/network.hpp 8234765e23bb3d434da0b0f818fd42569d554ab8
> 3rdparty/libprocess/include/process/posix/subprocess.hpp a1054e788ef6a322901c262380aceab8192235ac
> 3rdparty/libprocess/include/process/socket.hpp 87966155aa21328db51796b2ae0a883054c00457
> 3rdparty/libprocess/include/process/subprocess_base.hpp 74a4bef0706334b4d3c1040a08a8921fbc300344
> 3rdparty/libprocess/include/process/windows/subprocess.hpp 3bc7f1992d9c38dac2ec23d5bc57415f37d0318a
> 3rdparty/libprocess/src/encoder.hpp 1447d6f93c15b9f3d134507ecb0bda020a218a49
> 3rdparty/libprocess/src/http.cpp b5c38ce89b73788f7446e6c44fd99da6478b064d
> 3rdparty/libprocess/src/io.cpp 8aa3576a11ed5e998492b4020cb37a45ec708093
> 3rdparty/libprocess/src/libev_poll.cpp da2a78bd8aa4ed8c37bd2ca16d148b73112aa0e7
> 3rdparty/libprocess/src/libevent_poll.cpp 0803ba33622c86df38b3efd4f1e3197edf93a0af
> 3rdparty/libprocess/src/libevent_ssl_socket.hpp 57eaf4f607d0628db466cc1a139772eeeaa51136
> 3rdparty/libprocess/src/libevent_ssl_socket.cpp dddd0e292a8b0d470f4e199db08f09a0c863d73c
> 3rdparty/libprocess/src/poll_socket.hpp 89789e6bb91d79e4de1c4f4be3882df851845930
> 3rdparty/libprocess/src/poll_socket.cpp 93ca37f105527fb225574107480114ee5ac74c76
> 3rdparty/libprocess/src/process.cpp f475fe78f801924f70f51fdc4ab190c2dbecd656
> 3rdparty/libprocess/src/socket.cpp b819503095261c77f42d6f20d1a4b2b6170fb4e1
> 3rdparty/libprocess/src/subprocess.cpp ad19b0896b4a2e9c60f573cc854c10c69e909e86
> 3rdparty/libprocess/src/subprocess_posix.cpp 19271414f145d23f50ac07570c48782819f382b4
> 3rdparty/libprocess/src/subprocess_windows.cpp 20cad52d4a4d7fc51487e150a849972eb19ed08e
> 3rdparty/libprocess/src/tests/io_tests.cpp 466e343e6d775fe09debce119eae4fc4849b7006
> 3rdparty/libprocess/src/tests/ssl_tests.cpp a32d20e47f67d88bbe5928e0ddc129745c5f42e0
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp 59c17692012ddfb540ecdd48560c73c42a15f061
>
> Diff: https://reviews.apache.org/r/54602/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 54602: Replaced `int` with `int_fd` in libprocess.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54602/
-----------------------------------------------------------
(Updated Feb. 4, 2017, 5:39 p.m.)
Review request for mesos, Daniel Pravat and Joris Van Remoortere.
Changes
-------
Rebased.
Repository: mesos
Description
-------
Replaced `int` with `int_fd` in libprocess.
Diffs (updated)
-----
3rdparty/libprocess/include/process/io.hpp b342333bc7f2ae12b5b6a92fa21896c8f42353cb
3rdparty/libprocess/include/process/network.hpp 8234765e23bb3d434da0b0f818fd42569d554ab8
3rdparty/libprocess/include/process/posix/subprocess.hpp a1054e788ef6a322901c262380aceab8192235ac
3rdparty/libprocess/include/process/socket.hpp 5e958addc6012d85d1dd9025d27b258265807a9e
3rdparty/libprocess/include/process/subprocess_base.hpp 1c9f2e33a80e457857985d9b5663c8cb2a089505
3rdparty/libprocess/include/process/windows/subprocess.hpp f7beba16d6ed56be45bf5f6221e89a5b0cbcf1e7
3rdparty/libprocess/src/encoder.hpp 1447d6f93c15b9f3d134507ecb0bda020a218a49
3rdparty/libprocess/src/http.cpp f12a8a5d71f09c7413dae3e6192706ae63972923
3rdparty/libprocess/src/io.cpp e31f176a4ef110e9c7d0e5efd88c610e30718d2f
3rdparty/libprocess/src/libev_poll.cpp da2a78bd8aa4ed8c37bd2ca16d148b73112aa0e7
3rdparty/libprocess/src/libevent_poll.cpp 0803ba33622c86df38b3efd4f1e3197edf93a0af
3rdparty/libprocess/src/libevent_ssl_socket.hpp 97ce339c8183b4a4ffc4e37f052c9d2adb79654d
3rdparty/libprocess/src/libevent_ssl_socket.cpp fec131f912174e2f9c1b1060a45158d6a61c43c5
3rdparty/libprocess/src/poll_socket.hpp 89789e6bb91d79e4de1c4f4be3882df851845930
3rdparty/libprocess/src/poll_socket.cpp 93ca37f105527fb225574107480114ee5ac74c76
3rdparty/libprocess/src/process.cpp d1331238f6c9df47660bae8cdaa5e8c8add70212
3rdparty/libprocess/src/socket.cpp b819503095261c77f42d6f20d1a4b2b6170fb4e1
3rdparty/libprocess/src/subprocess.cpp b89dc7ebb86604fad7ca14a9750ef3faff7efbed
3rdparty/libprocess/src/subprocess_posix.cpp 19271414f145d23f50ac07570c48782819f382b4
3rdparty/libprocess/src/subprocess_windows.cpp 20cad52d4a4d7fc51487e150a849972eb19ed08e
3rdparty/libprocess/src/tests/io_tests.cpp 5c889e97cb1402a98b27cad2f3dbb4047a995506
3rdparty/libprocess/src/tests/ssl_tests.cpp a32d20e47f67d88bbe5928e0ddc129745c5f42e0
3rdparty/libprocess/src/tests/subprocess_tests.cpp 59c17692012ddfb540ecdd48560c73c42a15f061
Diff: https://reviews.apache.org/r/54602/diff/
Testing
-------
Thanks,
Michael Park