mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michael Park <mp...@apache.org>
Subject Re: Review Request 54602: Replaced `int` with `int_fd` in libprocess.
Date Sun, 05 Feb 2017 07:12:02 GMT


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


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