mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alex Clemmer <clemmer.alexan...@gmail.com>
Subject Re: Review Request 54602: Replaced `int` with `int_fd` in libprocess.
Date Thu, 02 Feb 2017 19:46:57 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message