Re: Review Request 38809: socket: refactor to use Option and fix file descriptor leaks.

[Joris Van Remoortere]

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



3rdparty/libprocess/src/socket.cpp (lines 79 - 81)


We're using `s.get()` above here, so `owned` should never be `true`?



3rdparty/libprocess/src/socket.cpp (lines 91 - 93)


We're using `s.get()` above here, so `owned` should never be `true`?


- Joris Van Remoortere


On Sept. 29, 2015, 8:30 p.m., Chi Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38809/
> ---
> 
> (Updated Sept. 29, 2015, 8:30 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3519
> https://issues.apache.org/jira/browse/MESOS-3519
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> socket: refactor to use Option and fix file descriptor leaks.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/socket.hpp 817cb37 
>   3rdparty/libprocess/src/socket.cpp 5879423 
> 
> Diff: https://reviews.apache.org/r/38809/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>

Re: Review Request 38809: socket: refactor to use Option and fix file descriptor leaks.

[Chi Zhang]

> On Sept. 29, 2015, 9:42 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/src/socket.cpp, lines 79-81
> > 
> >
> > We're using `s.get()` above here, so `owned` should never be `true`?

owned is determined upon entering the function? (we just reuse the variable 
name if s is none)


- Chi


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


On Sept. 29, 2015, 8:30 p.m., Chi Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38809/
> ---
> 
> (Updated Sept. 29, 2015, 8:30 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3519
> https://issues.apache.org/jira/browse/MESOS-3519
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> socket: refactor to use Option and fix file descriptor leaks.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/socket.hpp 817cb37 
>   3rdparty/libprocess/src/socket.cpp 5879423 
> 
> Diff: https://reviews.apache.org/r/38809/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>

Re: Review Request 38809: socket: refactor to use Option and fix file descriptor leaks.

[Joris Van Remoortere]

> On Sept. 29, 2015, 9:42 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/src/socket.cpp, lines 79-81
> > 
> >
> > We're using `s.get()` above here, so `owned` should never be `true`?
> 
> Chi Zhang wrote:
> owned is determined upon entering the function? (we just reuse the 
> variable name if s is none)

I see, it gets re-assigned.


- Joris


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


On Sept. 29, 2015, 8:30 p.m., Chi Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38809/
> ---
> 
> (Updated Sept. 29, 2015, 8:30 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3519
> https://issues.apache.org/jira/browse/MESOS-3519
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> socket: refactor to use Option and fix file descriptor leaks.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/socket.hpp 817cb37 
>   3rdparty/libprocess/src/socket.cpp 5879423 
> 
> Diff: https://reviews.apache.org/r/38809/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>

Re: Review Request 38809: socket: refactor to use Option and fix file descriptor leaks.

[Chi Zhang]

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

(Updated Sept. 29, 2015, 6:11 p.m.)


Review request for mesos and Ben Mahler.


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


Repository: mesos


Description
---

socket: refactor to use Option and fix file descriptor leaks.


Diffs (updated)
-

  3rdparty/libprocess/include/process/socket.hpp 817cb37 
  3rdparty/libprocess/src/socket.cpp 5879423 

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


Testing
---

make check


Thanks,

Chi Zhang

Re: Review Request 38809: socket: refactor to use Option and fix file descriptor leaks.

[Ben Mahler]

> On Sept. 29, 2015, 12:47 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/socket.cpp, lines 42-44
> > 
> >
> > Just as an aside, it's unfortunate the caller has to do this, we should 
> > consider doing the same thing that is done in os/posix/open.hpp with 
> > O_CLOEXEC:
> > 
> > 
> > https://github.com/apache/mesos/blob/5aa050bfaa7338b010b1a522406bde0f15015259/3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/open.hpp#L33
> 
> Chi Zhang wrote:
> I actually think this is not too bad -- a very staightforward if-else. 
> 
> I can probably break it up and save one network::socket call -- not sure 
> if that's worth the added complexity, but I can do it in another patch if 
> you'd like to see that?

No don't do it right now, just an aside. While it's straightforward here, every 
user of network::socket is going to have this #ifdef which complicates the 
calling code, it got out of hand for os::open which is why we provided 
O_CLOEXEC for all platforms.


- Ben


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


On Sept. 29, 2015, 6:11 p.m., Chi Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38809/
> ---
> 
> (Updated Sept. 29, 2015, 6:11 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3519
> https://issues.apache.org/jira/browse/MESOS-3519
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> socket: refactor to use Option and fix file descriptor leaks.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/socket.hpp 817cb37 
>   3rdparty/libprocess/src/socket.cpp 5879423 
> 
> Diff: https://reviews.apache.org/r/38809/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>

Re: Review Request 38809: socket: refactor to use Option and fix file descriptor leaks.

[Chi Zhang]

> On Sept. 29, 2015, 12:47 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/socket.cpp, lines 42-44
> > 
> >
> > Just as an aside, it's unfortunate the caller has to do this, we should 
> > consider doing the same thing that is done in os/posix/open.hpp with 
> > O_CLOEXEC:
> > 
> > 
> > https://github.com/apache/mesos/blob/5aa050bfaa7338b010b1a522406bde0f15015259/3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/open.hpp#L33
> 
> Chi Zhang wrote:
> I actually think this is not too bad -- a very staightforward if-else. 
> 
> I can probably break it up and save one network::socket call -- not sure 
> if that's worth the added complexity, but I can do it in another patch if 
> you'd like to see that?
> 
> Ben Mahler wrote:
> No don't do it right now, just an aside. While it's straightforward here, 
> every user of network::socket is going to have this #ifdef which complicates 
> the calling code, it got out of hand for os::open which is why we provided 
> O_CLOEXEC for all platforms.

okay, you meant pushing all this into network::socket. Yeah that makes sense.


- Chi


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


On Sept. 29, 2015, 8:30 p.m., Chi Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38809/
> ---
> 
> (Updated Sept. 29, 2015, 8:30 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3519
> https://issues.apache.org/jira/browse/MESOS-3519
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> socket: refactor to use Option and fix file descriptor leaks.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/socket.hpp 817cb37 
>   3rdparty/libprocess/src/socket.cpp 5879423 
> 
> Diff: https://reviews.apache.org/r/38809/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>

Re: Review Request 38809: socket: refactor to use Option and fix file descriptor leaks.

[Chi Zhang]

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

(Updated Sept. 29, 2015, 8:30 p.m.)


Review request for mesos and Ben Mahler.


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


Repository: mesos


Description
---

socket: refactor to use Option and fix file descriptor leaks.


Diffs (updated)
-

  3rdparty/libprocess/include/process/socket.hpp 817cb37 
  3rdparty/libprocess/src/socket.cpp 5879423 

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


Testing
---

make check


Thanks,

Chi Zhang

Re: Review Request 38809: socket: refactor to use Option and fix file descriptor leaks.

[Chi Zhang]

> On Sept. 29, 2015, 8:01 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/socket.cpp, line 42
> > 
> >
> > newline :)

you meant adding one above it right?


- Chi


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


On Sept. 29, 2015, 8:30 p.m., Chi Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38809/
> ---
> 
> (Updated Sept. 29, 2015, 8:30 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3519
> https://issues.apache.org/jira/browse/MESOS-3519
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> socket: refactor to use Option and fix file descriptor leaks.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/socket.hpp 817cb37 
>   3rdparty/libprocess/src/socket.cpp 5879423 
> 
> Diff: https://reviews.apache.org/r/38809/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>

Re: Review Request 38809: socket: refactor to use Option and fix file descriptor leaks.

[Ben Mahler]

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

Ship it!


Thanks Chi, just some minor cleanups to style. Once updated, I'll get this 
committed.


3rdparty/libprocess/src/socket.cpp (lines 39 - 40)


In general we try to wrap comments so as to avoid jaggedness, e.g.

```
  // If the caller passed in a file descriptor, we do not own its life
  // cycle and must not close it.
```

```
  // If the caller passed in a file descriptor, we do
  // not own its life cycle and must not close it.
```

The latter is easier on the eyes :)



3rdparty/libprocess/src/socket.cpp (line 42)


newline :)


- Ben Mahler


On Sept. 29, 2015, 6:11 p.m., Chi Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38809/
> ---
> 
> (Updated Sept. 29, 2015, 6:11 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3519
> https://issues.apache.org/jira/browse/MESOS-3519
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> socket: refactor to use Option and fix file descriptor leaks.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/socket.hpp 817cb37 
>   3rdparty/libprocess/src/socket.cpp 5879423 
> 
> Diff: https://reviews.apache.org/r/38809/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>

Re: Review Request 38809: socket: refactor to use Option and fix file descriptor leaks.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38809]

All tests passed.

- Mesos ReviewBot


On Sept. 28, 2015, 5:25 p.m., Chi Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38809/
> ---
> 
> (Updated Sept. 28, 2015, 5:25 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3519
> https://issues.apache.org/jira/browse/MESOS-3519
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> socket: refactor to use Option and fix file descriptor leaks.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/socket.hpp 
> a677e29b28711fc065134c11792b4f62fa3aa8b4 
>   3rdparty/libprocess/src/socket.cpp 5879423bd92c5805353a962cbd822a993b0568c5 
> 
> Diff: https://reviews.apache.org/r/38809/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>

Review Request 38809: socket: refactor to use Option and fix file descriptor leaks.

[Chi Zhang]

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

Review request for mesos and Ben Mahler.


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


Repository: mesos


Description
---

socket: refactor to use Option and fix file descriptor leaks.


Diffs
-

  3rdparty/libprocess/include/process/socket.hpp 
a677e29b28711fc065134c11792b4f62fa3aa8b4 
  3rdparty/libprocess/src/socket.cpp 5879423bd92c5805353a962cbd822a993b0568c5 

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


Testing
---


Thanks,

Chi Zhang

Re: Review Request 38809: socket: refactor to use Option and fix file descriptor leaks.

[Neil Conway]

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



3rdparty/libprocess/src/socket.cpp (line 40)


"socketfd.isNone()" would be more readable, IMHO.



3rdparty/libprocess/src/socket.cpp (line 77)


Why is this conditional on socketFd?



3rdparty/libprocess/src/socket.cpp (line 89)


Same as above.


- Neil Conway


On Sept. 28, 2015, 5:25 p.m., Chi Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38809/
> ---
> 
> (Updated Sept. 28, 2015, 5:25 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3519
> https://issues.apache.org/jira/browse/MESOS-3519
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> socket: refactor to use Option and fix file descriptor leaks.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/socket.hpp 
> a677e29b28711fc065134c11792b4f62fa3aa8b4 
>   3rdparty/libprocess/src/socket.cpp 5879423bd92c5805353a962cbd822a993b0568c5 
> 
> Diff: https://reviews.apache.org/r/38809/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>

Re: Review Request 38809: socket: refactor to use Option and fix file descriptor leaks.

[Neil Conway]

> On Sept. 28, 2015, 6:03 p.m., Neil Conway wrote:
> > 3rdparty/libprocess/src/socket.cpp, line 77
> > 
> >
> > Why is this conditional on socketFd?
> 
> Chi Zhang wrote:
> The idea is I am only responsible for the resource I allocated. If a 
> socketfd is passed in, I leave it to the owner to free it; if a socketfd is 
> created by me, I will free it.

Duh, makes sense.


- Neil


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


On Sept. 28, 2015, 9:07 p.m., Chi Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38809/
> ---
> 
> (Updated Sept. 28, 2015, 9:07 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3519
> https://issues.apache.org/jira/browse/MESOS-3519
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> socket: refactor to use Option and fix file descriptor leaks.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/socket.hpp 
> 817cb3711742871630a13b966563296644008f21 
>   3rdparty/libprocess/src/socket.cpp 5879423bd92c5805353a962cbd822a993b0568c5 
> 
> Diff: https://reviews.apache.org/r/38809/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>

Re: Review Request 38809: socket: refactor to use Option and fix file descriptor leaks.

[Chi Zhang]

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

(Updated Sept. 28, 2015, 9:07 p.m.)


Review request for mesos and Ben Mahler.


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


Repository: mesos


Description
---

socket: refactor to use Option and fix file descriptor leaks.


Diffs (updated)
-

  3rdparty/libprocess/include/process/socket.hpp 
817cb3711742871630a13b966563296644008f21 
  3rdparty/libprocess/src/socket.cpp 5879423bd92c5805353a962cbd822a993b0568c5 

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


Testing (updated)
---

make check


Thanks,

Chi Zhang

Re: Review Request 38809: socket: refactor to use Option and fix file descriptor leaks.

[Ben Mahler]

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



3rdparty/libprocess/include/process/socket.hpp (lines 64 - 65)


Can you fix the wrapping here? Also, I would suggest s/socketfd/s/ to keep 
the same as before, since `socketfd` is a bit unusual for our code base. Also 
the doxygen a few lines up refers to 's' still :)



3rdparty/libprocess/src/socket.cpp (lines 42 - 44)


Just as an aside, it's unfortunate the caller has to do this, we should 
consider doing the same thing that is done in os/posix/open.hpp with O_CLOEXEC:


https://github.com/apache/mesos/blob/5aa050bfaa7338b010b1a522406bde0f15015259/3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/open.hpp#L33



3rdparty/libprocess/src/socket.cpp (lines 77 - 79)


How about we store a bool for this at the top instead of having multiple 
variables?

```
// If the caller passed in a descriptor, we do
// not own its lifecycle and must not close it!
bool owned = s.isNone();

...
```


- Ben Mahler


On Sept. 28, 2015, 9:07 p.m., Chi Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38809/
> ---
> 
> (Updated Sept. 28, 2015, 9:07 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3519
> https://issues.apache.org/jira/browse/MESOS-3519
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> socket: refactor to use Option and fix file descriptor leaks.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/socket.hpp 
> 817cb3711742871630a13b966563296644008f21 
>   3rdparty/libprocess/src/socket.cpp 5879423bd92c5805353a962cbd822a993b0568c5 
> 
> Diff: https://reviews.apache.org/r/38809/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>

Re: Review Request 38809: socket: refactor to use Option and fix file descriptor leaks.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38809]

All tests passed.

- Mesos ReviewBot


On Sept. 28, 2015, 9:07 p.m., Chi Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38809/
> ---
> 
> (Updated Sept. 28, 2015, 9:07 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3519
> https://issues.apache.org/jira/browse/MESOS-3519
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> socket: refactor to use Option and fix file descriptor leaks.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/socket.hpp 
> 817cb3711742871630a13b966563296644008f21 
>   3rdparty/libprocess/src/socket.cpp 5879423bd92c5805353a962cbd822a993b0568c5 
> 
> Diff: https://reviews.apache.org/r/38809/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>