----------------------------------------------------------- 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 > >
