Re: Review Request 53802: Eliminated an EOF race condition in libprocess SSL socket.

[Greg Mann]

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

(Updated Jan. 11, 2017, 12:51 a.m.)


Review request for mesos, Benjamin Hindman, Benjamin Mahler, and Joseph Wu.


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


Repository: mesos


Description
---

Previously, it was possible for an SSL socket to either:
1) Fail to receive an EOF if the EOF event was received when
   there was no pending recv() request.
2) Fail to receive all data sent on the sending side if an
   EOF event was received before all sent data was read.

This patch eliminates these race conditions to ensure reliable
receipt of both sent data and EOFs.


Diffs (updated)
-

  3rdparty/libprocess/src/libevent_ssl_socket.hpp 
57eaf4f607d0628db466cc1a139772eeeaa51136 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
0e292a8b0d470f4e199db08f09a0c863d73c 

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


Testing
---

Testing details are found at the end of this chain.


Thanks,

Greg Mann

Re: Review Request 53802: Eliminated an EOF race condition in libprocess SSL socket.

[Greg Mann]

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

(Updated Jan. 11, 2017, 12:11 a.m.)


Review request for mesos, Benjamin Mahler and Joseph Wu.


Changes
---

Addressed BenH's comments, implemented approach with less churn.


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


Repository: mesos


Description
---

Previously, it was possible for an SSL socket to either:
1) Fail to receive an EOF if the EOF event was received when
   there was no pending recv() request.
2) Fail to receive all data sent on the sending side if an
   EOF event was received before all sent data was read.

This patch eliminates these race conditions to ensure reliable
receipt of both sent data and EOFs.


Diffs (updated)
-

  3rdparty/libprocess/src/libevent_ssl_socket.hpp 
57eaf4f607d0628db466cc1a139772eeeaa51136 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
0e292a8b0d470f4e199db08f09a0c863d73c 

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


Testing
---

Testing details are found at the end of this chain.


Thanks,

Greg Mann

Re: Review Request 53802: Eliminated an EOF race condition in libprocess SSL socket.

[Greg Mann]

> On Jan. 8, 2017, 3:22 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/src/libevent_ssl_socket.hpp, line 143
> > 
> >
> > I don't see any win in the name change from `recv_callback` to 
> > `perform_bev_read`. You also didn't delete the declaration of 
> > `recv_callback` above even though you changed the function name below.

I liked the name perform_bev_read because this function is no longer simply a 
continuation of the libevent callback, it also has a callsite in 
event_callback. I also find it confusing that the XXX_callback functions all 
have continuations with the same name in this file.

However, you make a good point regarding consistency. We could rename this 
function `_recv_callback`, which matches our usual pattern for continuations, 
and I could follow up with another patch to similarly change the names of the 
relevant `send_callback` and `event_callback` functions. Thoughts?


> On Jan. 8, 2017, 3:22 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/src/libevent_ssl_socket.hpp, line 155
> > 
> >
> > Was your inititon that `received_eof` was going to be protected by 
> > `synchronized (bev)`? I want to make sure there isn't a race with the IO 
> > thread setting `received_eof` in the `event_callback` and another thread 
> > reading and missing `received_eof` in `recv`. If you're confident that the 
> > `synchronized (bev)` is a sufficient barrier then let's just document that 
> > and maybe even move this up closer to `bufferevent* bev;` and specify that 
> > it's protected by `synchronized (bev)` which it gets automatically in any 
> > of the callbacks (i.e., `recv_callback`, `send_callback`, `event_callback`).

I originally did have received_eof explicitly protected by synchronized (bev), 
but as BenM mentioned in our offline discussion we should be safe without this 
since all accesses of received_eof occur in the event loop. I'll move this 
declaration and add a comment.


- Greg


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


On Dec. 15, 2016, 5:53 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53802/
> ---
> 
> (Updated Dec. 15, 2016, 5:53 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Joseph Wu.
> 
> 
> Bugs: MESOS-6802
> https://issues.apache.org/jira/browse/MESOS-6802
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, it was possible for an SSL socket to either:
> 1) Fail to receive an EOF if the EOF event was received when
>there was no pending recv() request.
> 2) Fail to receive all data sent on the sending side if an
>EOF event was received before all sent data was read.
> 
> This patch eliminates these race conditions to ensure reliable
> receipt of both sent data and EOFs.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 
> 57eaf4f607d0628db466cc1a139772eeeaa51136 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 0e292a8b0d470f4e199db08f09a0c863d73c 
> 
> Diff: https://reviews.apache.org/r/53802/diff/
> 
> 
> Testing
> ---
> 
> Testing details are found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 53802: Eliminated an EOF race condition in libprocess SSL socket.

[Greg Mann]

> On Jan. 9, 2017, 11:54 p.m., Greg Mann wrote:
> >

Whoops :)


- Greg


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


On Dec. 15, 2016, 5:53 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53802/
> ---
> 
> (Updated Dec. 15, 2016, 5:53 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Joseph Wu.
> 
> 
> Bugs: MESOS-6802
> https://issues.apache.org/jira/browse/MESOS-6802
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, it was possible for an SSL socket to either:
> 1) Fail to receive an EOF if the EOF event was received when
>there was no pending recv() request.
> 2) Fail to receive all data sent on the sending side if an
>EOF event was received before all sent data was read.
> 
> This patch eliminates these race conditions to ensure reliable
> receipt of both sent data and EOFs.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 
> 57eaf4f607d0628db466cc1a139772eeeaa51136 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 0e292a8b0d470f4e199db08f09a0c863d73c 
> 
> Diff: https://reviews.apache.org/r/53802/diff/
> 
> 
> Testing
> ---
> 
> Testing details are found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 53802: Eliminated an EOF race condition in libprocess SSL socket.

[Greg Mann]

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




3rdparty/libprocess/src/libevent_ssl_socket.hpp (line 143)


I liked the name `perform_bev_read` because this function is no longer 
simply a continuation of the libevent callback, it also has a callsite in 
`event_callback`. I also find it confusing that the `XXX_callback` functions 
all have continuations with the same name in this file.

However, you make a good point regarding consistency. We could rename this 
function `_recv_callback`, which matches our usual pattern for continuations, 
and I could follow up with another patch to similarly change the names of the 
relevant `send_callback` and `event_callback` functions. Thoughts?



3rdparty/libprocess/src/libevent_ssl_socket.hpp (line 155)


I originally did have `received_eof` explicitly protected by `synchronized 
(bev)`, but as BenM mentioned in our offline discussion we should be safe 
without this since all accesses of `received_eof` occur in the event loop. I'll 
move this declaration and add a comment.


- Greg Mann


On Dec. 15, 2016, 5:53 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53802/
> ---
> 
> (Updated Dec. 15, 2016, 5:53 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Joseph Wu.
> 
> 
> Bugs: MESOS-6802
> https://issues.apache.org/jira/browse/MESOS-6802
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, it was possible for an SSL socket to either:
> 1) Fail to receive an EOF if the EOF event was received when
>there was no pending recv() request.
> 2) Fail to receive all data sent on the sending side if an
>EOF event was received before all sent data was read.
> 
> This patch eliminates these race conditions to ensure reliable
> receipt of both sent data and EOFs.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 
> 57eaf4f607d0628db466cc1a139772eeeaa51136 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 0e292a8b0d470f4e199db08f09a0c863d73c 
> 
> Diff: https://reviews.apache.org/r/53802/diff/
> 
> 
> Testing
> ---
> 
> Testing details are found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 53802: Eliminated an EOF race condition in libprocess SSL socket.

[Benjamin Hindman]

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


Fix it, then Ship it!





3rdparty/libprocess/src/libevent_ssl_socket.hpp (line 143)


I don't see any win in the name change from `recv_callback` to 
`perform_bev_read`. You also didn't delete the declaration of `recv_callback` 
above even though you changed the function name below.



3rdparty/libprocess/src/libevent_ssl_socket.hpp (line 155)


Was your inititon that `received_eof` was going to be protected by 
`synchronized (bev)`? I want to make sure there isn't a race with the IO thread 
setting `received_eof` in the `event_callback` and another thread reading and 
missing `received_eof` in `recv`. If you're confident that the `synchronized 
(bev)` is a sufficient barrier then let's just document that and maybe even 
move this up closer to `bufferevent* bev;` and specify that it's protected by 
`synchronized (bev)` which it gets automatically in any of the callbacks (i.e., 
`recv_callback`, `send_callback`, `event_callback`).



3rdparty/libprocess/src/libevent_ssl_socket.cpp (line 228)


One of the rasons I prefer calling this `recv_callback` is that it has 
symmetry with the others, `send_callback` and `event_callback`.



3rdparty/libprocess/src/libevent_ssl_socket.cpp (lines 399 - 402)


This is a tricky comment. Here's my iteration on what you have to try and 
call out that even though we've received EOF there still might be data left in 
the buffer:

While we've received EOF there is still the possibility that we have data 
left in the buffer that needs to get drained first. Thus, we pretend as though 
we've just received data and force a `recv_callback` explicitly which will 
either fulfill an existing `recv_request` with the data or if there is no data 
in the buffer it will complete the `recv_request` with a length of zero, 
representing EOF.


- Benjamin Hindman


On Dec. 15, 2016, 5:53 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53802/
> ---
> 
> (Updated Dec. 15, 2016, 5:53 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Joseph Wu.
> 
> 
> Bugs: MESOS-6802
> https://issues.apache.org/jira/browse/MESOS-6802
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, it was possible for an SSL socket to either:
> 1) Fail to receive an EOF if the EOF event was received when
>there was no pending recv() request.
> 2) Fail to receive all data sent on the sending side if an
>EOF event was received before all sent data was read.
> 
> This patch eliminates these race conditions to ensure reliable
> receipt of both sent data and EOFs.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 
> 57eaf4f607d0628db466cc1a139772eeeaa51136 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 0e292a8b0d470f4e199db08f09a0c863d73c 
> 
> Diff: https://reviews.apache.org/r/53802/diff/
> 
> 
> Testing
> ---
> 
> Testing details are found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 53802: Eliminated an EOF race condition in libprocess SSL socket.

[Greg Mann]

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

(Updated Dec. 15, 2016, 5:53 p.m.)


Review request for mesos, Benjamin Mahler and Joseph Wu.


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


Repository: mesos


Description
---

Previously, it was possible for an SSL socket to either:
1) Fail to receive an EOF if the EOF event was received when
   there was no pending recv() request.
2) Fail to receive all data sent on the sending side if an
   EOF event was received before all sent data was read.

This patch eliminates these race conditions to ensure reliable
receipt of both sent data and EOFs.


Diffs
-

  3rdparty/libprocess/src/libevent_ssl_socket.hpp 
57eaf4f607d0628db466cc1a139772eeeaa51136 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
0e292a8b0d470f4e199db08f09a0c863d73c 

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


Testing
---

Testing details are found at the end of this chain.


Thanks,

Greg Mann

Re: Review Request 53802: Eliminated an EOF race condition in libprocess SSL socket.

[Greg Mann]

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

(Updated Dec. 10, 2016, 12:33 a.m.)


Review request for mesos, Benjamin Mahler and Joseph Wu.


Changes
---

Fixed a conditional.


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


Repository: mesos


Description
---

Previously, it was possible for an SSL socket to either:
1) Fail to receive an EOF if the EOF event was received when
   there was no pending recv() request.
2) Fail to receive all data sent on the sending side if an
   EOF event was received before all sent data was read.

This patch eliminates these race conditions to ensure reliable
receipt of both sent data and EOFs.


Diffs (updated)
-

  3rdparty/libprocess/src/libevent_ssl_socket.hpp 
57eaf4f607d0628db466cc1a139772eeeaa51136 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
0e292a8b0d470f4e199db08f09a0c863d73c 

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


Testing
---

Testing details are found at the end of this chain.


Thanks,

Greg Mann

Re: Review Request 53802: Eliminated an EOF race condition in libprocess SSL socket.

[Greg Mann]

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

(Updated Dec. 9, 2016, 9:26 p.m.)


Review request for mesos, Benjamin Mahler and Joseph Wu.


Changes
---

Linked to a JIRA in a comment.


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


Repository: mesos


Description
---

Previously, it was possible for an SSL socket to either:
1) Fail to receive an EOF if the EOF event was received when
   there was no pending recv() request.
2) Fail to receive all data sent on the sending side if an
   EOF event was received before all sent data was read.

This patch eliminates these race conditions to ensure reliable
receipt of both sent data and EOFs.


Diffs (updated)
-

  3rdparty/libprocess/src/libevent_ssl_socket.hpp 
57eaf4f607d0628db466cc1a139772eeeaa51136 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
0e292a8b0d470f4e199db08f09a0c863d73c 

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


Testing
---

Testing details are found at the end of this chain.


Thanks,

Greg Mann

Re: Review Request 53802: Eliminated an EOF race condition in libprocess SSL socket.

[Greg Mann]

> On Dec. 9, 2016, 1 a.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/src/libevent_ssl_socket.cpp, lines 366-367
> > 
> >
> > Let's remove this part of the case:
> > 
> > ```
> > events & BEV_EVENT_ERROR && EVUTIL_SOCKET_ERROR() == 0)
> > ```
> > 
> > and take the CHECK out of the final error case. We can just call 
> > evutil_socket_error_to_string even if it's 0 AFAICT (although you should 
> > check on windows if it's ok).
> 
> Greg Mann wrote:
> I checked out the windows code in libevent and calling 
> `evutil_socket_error_to_string(0)` looks safe:
> 
> ```
> const char *
> evutil_socket_error_to_string(int errcode)
> {
>   /*  Is there really no built-in function to do this? */
>   int i;
>   for (i=0; windows_socket_errors[i].code >= 0; ++i) {
> if (errcode == windows_socket_errors[i].code)
>   return windows_socket_errors[i].msg;
>   }
>   return strerror(errcode);
> }
> ```

After removing the extra conditional, my new SocketEOF tests are throwing an 
error because they are reaching the error-handling code in the event callback, 
instead of the EOF-handling code. It would seem that checking for `events & 
BEV_EVENT_ERROR && EVUTIL_SOCKET_ERROR() == 0` is necessary to detect EOF, but 
I'm not sure why :(


- Greg


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


On Dec. 9, 2016, 9:19 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53802/
> ---
> 
> (Updated Dec. 9, 2016, 9:19 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Joseph Wu.
> 
> 
> Bugs: MESOS-5966
> https://issues.apache.org/jira/browse/MESOS-5966
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, it was possible for an SSL socket to either:
> 1) Fail to receive an EOF if the EOF event was received when
>there was no pending recv() request.
> 2) Fail to receive all data sent on the sending side if an
>EOF event was received before all sent data was read.
> 
> This patch eliminates these race conditions to ensure reliable
> receipt of both sent data and EOFs.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 
> 57eaf4f607d0628db466cc1a139772eeeaa51136 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 0e292a8b0d470f4e199db08f09a0c863d73c 
> 
> Diff: https://reviews.apache.org/r/53802/diff/
> 
> 
> Testing
> ---
> 
> Testing details are found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 53802: Eliminated an EOF race condition in libprocess SSL socket.

[Greg Mann]

> On Dec. 9, 2016, 7:45 p.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/src/libevent_ssl_socket.cpp, lines 400-402
> > 
> >
> > Any reason this is in the locked section? The implication of it being 
> > locked it that there are accesses off of the event loop thread which does 
> > not appear to be the case?

Ah, good point! I was thrown off by the access which occurs in `recv()`, but 
it's in a block that is executed on the event loop so it's safe. Thx!


> On Dec. 9, 2016, 7:45 p.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/src/libevent_ssl_socket.cpp, line 349
> > 
> >
> > Not yours, but what is a "valid" error?

I decided to simply remove this comment, since the code is pretty 
self-explanatory.


- Greg


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


On Dec. 9, 2016, 9:19 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53802/
> ---
> 
> (Updated Dec. 9, 2016, 9:19 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Joseph Wu.
> 
> 
> Bugs: MESOS-5966
> https://issues.apache.org/jira/browse/MESOS-5966
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, it was possible for an SSL socket to either:
> 1) Fail to receive an EOF if the EOF event was received when
>there was no pending recv() request.
> 2) Fail to receive all data sent on the sending side if an
>EOF event was received before all sent data was read.
> 
> This patch eliminates these race conditions to ensure reliable
> receipt of both sent data and EOFs.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 
> 57eaf4f607d0628db466cc1a139772eeeaa51136 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 0e292a8b0d470f4e199db08f09a0c863d73c 
> 
> Diff: https://reviews.apache.org/r/53802/diff/
> 
> 
> Testing
> ---
> 
> Testing details are found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 53802: Eliminated an EOF race condition in libprocess SSL socket.

[Greg Mann]

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

(Updated Dec. 9, 2016, 9:19 p.m.)


Review request for mesos, Benjamin Mahler and Joseph Wu.


Changes
---

Addressed BenM's comments.


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


Repository: mesos


Description
---

Previously, it was possible for an SSL socket to either:
1) Fail to receive an EOF if the EOF event was received when
   there was no pending recv() request.
2) Fail to receive all data sent on the sending side if an
   EOF event was received before all sent data was read.

This patch eliminates these race conditions to ensure reliable
receipt of both sent data and EOFs.


Diffs (updated)
-

  3rdparty/libprocess/src/libevent_ssl_socket.hpp 
57eaf4f607d0628db466cc1a139772eeeaa51136 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
0e292a8b0d470f4e199db08f09a0c863d73c 

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


Testing
---

Testing details are found at the end of this chain.


Thanks,

Greg Mann

Re: Review Request 53802: Eliminated an EOF race condition in libprocess SSL socket.

[Benjamin Mahler]

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


Fix it, then Ship it!




Looks good to me. I just added another suggestion to handle BEV_EVENT_READING 
and BEV_EVENT_WRITING errors independently in a separate patch.


3rdparty/libprocess/src/libevent_ssl_socket.cpp (lines 332 - 334)


I commented below about the "or it has been discarded" when it comes to the 
CHECK on the connect request. As far as I can tell, there is only discard 
handling for recv.



3rdparty/libprocess/src/libevent_ssl_socket.cpp (line 336)


Can you add a TODO here to handle the BEV_EVENT_READING and 
BEV_EVENT_WRITING errors independently? Right now we treat a read error as a 
write error and vice versa, since we treat both in the same way.

I think we're ok in this patch, you can do this separately. We should also 
assert that we don't see BEV_EVENT_TIMEOUT here, since we're not using that 
functionality.



3rdparty/libprocess/src/libevent_ssl_socket.cpp (line 340)


Not yours, but what is a "valid" error?



3rdparty/libprocess/src/libevent_ssl_socket.cpp (lines 388 - 390)


Any reason this is in the locked section? The implication of it being 
locked it that there are accesses off of the event loop thread which does not 
appear to be the case?



3rdparty/libprocess/src/libevent_ssl_socket.cpp (line 417)


The comment above about requests being null seems to suggest that the 
connect request could be null if it was discarded? But looking at the code it 
seems we don't handle discard for connect requests so this can't be null? Might 
want to clarify this briefly on this CHECK.


- Benjamin Mahler


On Dec. 9, 2016, 7:10 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53802/
> ---
> 
> (Updated Dec. 9, 2016, 7:10 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Joseph Wu.
> 
> 
> Bugs: MESOS-5966
> https://issues.apache.org/jira/browse/MESOS-5966
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, it was possible for an SSL socket to either:
> 1) Fail to receive an EOF if the EOF event was received when
>there was no pending recv() request.
> 2) Fail to receive all data sent on the sending side if an
>EOF event was received before all sent data was read.
> 
> This patch eliminates these race conditions to ensure reliable
> receipt of both sent data and EOFs.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 
> 57eaf4f607d0628db466cc1a139772eeeaa51136 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 0e292a8b0d470f4e199db08f09a0c863d73c 
> 
> Diff: https://reviews.apache.org/r/53802/diff/
> 
> 
> Testing
> ---
> 
> Testing details are found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 53802: Eliminated an EOF race condition in libprocess SSL socket.

[Greg Mann]

> On Dec. 9, 2016, 1 a.m., Benjamin Mahler wrote:
> > I went over this with Greg.
> > 
> > This looks good, some suggestions around some of the existing logic that we 
> > should fix.
> > 
> > Also, some other items:
> > 
> > (1) It looks like the shutdown override is no longer overriding. The code 
> > in shutdown() looks unnecessary? Do we need to clear buffers or something?
> > (2) It looks like we don't need the SSL_set_shutdown workaround code since 
> > we don't use BEV_OPT_CLOSE_ON_FREE?

Thanks for the review, Ben! I've addressed the comments below, and I'll submit 
follow-up patches for your two points above, as well as for the calls to 
`EVUTIL_SOCKET_ERROR` in `accept_SSL_callback`.


> On Dec. 9, 2016, 1 a.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/src/libevent_ssl_socket.cpp, lines 366-367
> > 
> >
> > Let's remove this part of the case:
> > 
> > ```
> > events & BEV_EVENT_ERROR && EVUTIL_SOCKET_ERROR() == 0)
> > ```
> > 
> > and take the CHECK out of the final error case. We can just call 
> > evutil_socket_error_to_string even if it's 0 AFAICT (although you should 
> > check on windows if it's ok).

I checked out the windows code in libevent and calling 
`evutil_socket_error_to_string(0)` looks safe:

```
const char *
evutil_socket_error_to_string(int errcode)
{
  /*  Is there really no built-in function to do this? */
  int i;
  for (i=0; windows_socket_errors[i].code >= 0; ++i) {
if (errcode == windows_socket_errors[i].code)
  return windows_socket_errors[i].msg;
  }
  return strerror(errcode);
}
```


> On Dec. 9, 2016, 1 a.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/src/libevent_ssl_socket.cpp, lines 417-425
> > 
> >
> > Should we also pull out the CHECKs for recv and send requests being 
> > null? It seems we actually allow the caller to do this, and the idea being 
> > CHECKs is generally to check our internal invariants. So the caller could 
> > induce a CHECK failure here by calling send() without waiting for a connect 
> > to finish.
> > 
> > Unfortunately, I'm not sure how libevent handles the case where the 
> > socket is not connected and we write to its buffer (hopefully that would 
> > give us a EVUTIL_SOCKET_ERROR back in the event_callback).

OK, I removed the null checks. As we discovered yesterday in the libevent 
documentation, writing to the buffer before the connection is established is 
actually allowed: "It is okay to add data to the output buffer before the 
connect is done." 
(http://www.wangafu.net/~nickm/libevent-book/Ref6_bufferevent.html)


> On Dec. 9, 2016, 1 a.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/src/libevent_ssl_socket.cpp, line 367
> > 
> >
> > It's not clear if this case is possible. At least looking at the 
> > documentation and some of the example code it seems to suggest that we are 
> > expected to see a non-zero EVUTIL_SOCKET_ERROR whenever BEV_EVENT_ERROR is 
> > set. This is because EVUTIL_SOCKET_ERROR just returns errno (or 
> > WSAGetLastError on windows) and unless libevent is clearing these it 
> > wouldn't be safe to look at errno since it might still be set to a stale 
> > value.
> > 
> > Also, libevent says that EVUTIL_SOCKET_ERROR isn't idempotent on all 
> > platforms so we should pull this out into a variable.

Since I've removed the `events & BEV_EVENT_ERROR && EVUTIL_SOCKET_ERROR() == 0` 
check, this function has only a single invocation of `EVUTIL_SOCKET_ERROR`. For 
this reason, I moved the `if` branch which calls `EVUTIL_SOCKET_ERROR` up to 
the top so that we invoke the macro ASAP without the need to store it in a 
local variable.


- Greg


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


On Dec. 9, 2016, 7:10 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53802/
> ---
> 
> (Updated Dec. 9, 2016, 7:10 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Joseph Wu.
> 
> 
> Bugs: MESOS-5966
> https://issues.apache.org/jira/browse/MESOS-5966
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, it was possible for an SSL socket to either:
> 1) Fail to receive an EOF if the EOF event was received when
>there was no pending recv() request.
> 2) Fail to receive all data sent on the sending side if an
>EOF event was received before all sent data was read.
> 
> This patch eliminates these race conditions to ensure 

Re: Review Request 53802: Eliminated an EOF race condition in libprocess SSL socket.

[Greg Mann]

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

(Updated Dec. 9, 2016, 7:10 p.m.)


Review request for mesos, Benjamin Mahler and Joseph Wu.


Changes
---

Addressed BenM's comments.


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


Repository: mesos


Description
---

Previously, it was possible for an SSL socket to either:
1) Fail to receive an EOF if the EOF event was received when
   there was no pending recv() request.
2) Fail to receive all data sent on the sending side if an
   EOF event was received before all sent data was read.

This patch eliminates these race conditions to ensure reliable
receipt of both sent data and EOFs.


Diffs (updated)
-

  3rdparty/libprocess/src/libevent_ssl_socket.hpp 
57eaf4f607d0628db466cc1a139772eeeaa51136 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
0e292a8b0d470f4e199db08f09a0c863d73c 

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


Testing
---

Testing details are found at the end of this chain.


Thanks,

Greg Mann

Re: Review Request 53802: Eliminated an EOF race condition in libprocess SSL socket.

[Greg Mann]

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



Also consider adding `override` to the implementation-specific `Socket` 
overrides to avoid other situations like this `Socket::shutdown` issue in the 
future.

- Greg Mann


On Nov. 30, 2016, 11:12 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53802/
> ---
> 
> (Updated Nov. 30, 2016, 11:12 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Joseph Wu.
> 
> 
> Bugs: MESOS-5966
> https://issues.apache.org/jira/browse/MESOS-5966
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, it was possible for an SSL socket to either:
> 1) Fail to receive an EOF if the EOF event was received when
>there was no pending recv() request.
> 2) Fail to receive all data sent on the sending side if an
>EOF event was received before all sent data was read.
> 
> This patch eliminates these race conditions to ensure reliable
> receipt of both sent data and EOFs.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 
> 57eaf4f607d0628db466cc1a139772eeeaa51136 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 0e292a8b0d470f4e199db08f09a0c863d73c 
> 
> Diff: https://reviews.apache.org/r/53802/diff/
> 
> 
> Testing
> ---
> 
> Testing details are found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 53802: Eliminated an EOF race condition in libprocess SSL socket.

[Benjamin Mahler]

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



I went over this with Greg.

This looks good, some suggestions around some of the existing logic that we 
should fix.

Also, some other items:

(1) It looks like the shutdown override is no longer overriding. The code in 
shutdown() looks unnecessary? Do we need to clear buffers or something?
(2) It looks like we don't need the SSL_set_shutdown workaround code since we 
don't use BEV_OPT_CLOSE_ON_FREE?


3rdparty/libprocess/src/libevent_ssl_socket.cpp (lines 250 - 252)


This seems a bit misleading since this is necessary for both callers? Maybe 
just avoid this comment?



3rdparty/libprocess/src/libevent_ssl_socket.cpp (lines 260 - 276)


Do we still need this comment in the buffer_length > 0 case? Seems 
inconsistent with the long comment above?

```
if (buffer_length > 0) {
  size_t length = bufferevent_read(bev, request->data, request->size);
  CHECK(length > 0);
  request->promise.set(length);
} else {
  CHECK(received_eof);
  request->promise.set(0);
}
```



3rdparty/libprocess/src/libevent_ssl_socket.cpp (lines 281 - 292)


Looks like we can consolidate `recv_callback()` and `perform_bev_read()`? 
I'd be inclined to remove this `recv_callback()` in favor of calling 
`perform_bev_read()` from our call-sites and the `recv_callback(bufferevent*, 
void*)` implementation.



3rdparty/libprocess/src/libevent_ssl_socket.cpp (lines 362 - 363)


Let's remove this part of the case:

```
events & BEV_EVENT_ERROR && EVUTIL_SOCKET_ERROR() == 0)
```

and take the CHECK out of the final error case. We can just call 
evutil_socket_error_to_string even if it's 0 AFAICT (although you should check 
on windows if it's ok).



3rdparty/libprocess/src/libevent_ssl_socket.cpp (line 363)


It's not clear if this case is possible. At least looking at the 
documentation and some of the example code it seems to suggest that we are 
expected to see a non-zero EVUTIL_SOCKET_ERROR whenever BEV_EVENT_ERROR is set. 
This is because EVUTIL_SOCKET_ERROR just returns errno (or WSAGetLastError on 
windows) and unless libevent is clearing these it wouldn't be safe to look at 
errno since it might still be set to a stale value.

Also, libevent says that EVUTIL_SOCKET_ERROR isn't idempotent on all 
platforms so we should pull this out into a variable.



3rdparty/libprocess/src/libevent_ssl_socket.cpp (lines 402 - 410)


Should we also pull out the CHECKs for recv and send requests being null? 
It seems we actually allow the caller to do this, and the idea being CHECKs is 
generally to check our internal invariants. So the caller could induce a CHECK 
failure here by calling send() without waiting for a connect to finish.

Unfortunately, I'm not sure how libevent handles the case where the socket 
is not connected and we write to its buffer (hopefully that would give us a 
EVUTIL_SOCKET_ERROR back in the event_callback).



3rdparty/libprocess/src/libevent_ssl_socket.cpp (lines 699 - 709)


We could probably do away with the comment here, should we call 
`perform_bev_read()` instead of the callback?


- Benjamin Mahler


On Nov. 30, 2016, 11:12 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53802/
> ---
> 
> (Updated Nov. 30, 2016, 11:12 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Joseph Wu.
> 
> 
> Bugs: MESOS-5966
> https://issues.apache.org/jira/browse/MESOS-5966
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, it was possible for an SSL socket to either:
> 1) Fail to receive an EOF if the EOF event was received when
>there was no pending recv() request.
> 2) Fail to receive all data sent on the sending side if an
>EOF event was received before all sent data was read.
> 
> This patch eliminates these race conditions to ensure reliable
> receipt of both sent data and EOFs.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 
> 57eaf4f607d0628db466cc1a139772eeeaa51136 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 0e292a8b0d470f4e199db08f09a0c863d73c 
> 
> Diff: https://reviews.apache.org/r/53802/diff/
> 
> 
> Testing
> ---
> 
> Testing details are found at the end 

Re: Review Request 53802: Eliminated an EOF race condition in libprocess SSL socket.

[Greg Mann]

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

(Updated Nov. 30, 2016, 11:12 p.m.)


Review request for mesos, Benjamin Mahler and Joseph Wu.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Previously, it was possible for an SSL socket to either:
1) Fail to receive an EOF if the EOF event was received when
   there was no pending recv() request.
2) Fail to receive all data sent on the sending side if an
   EOF event was received before all sent data was read.

This patch eliminates these race conditions to ensure reliable
receipt of both sent data and EOFs.


Diffs (updated)
-

  3rdparty/libprocess/src/libevent_ssl_socket.hpp 
57eaf4f607d0628db466cc1a139772eeeaa51136 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
0e292a8b0d470f4e199db08f09a0c863d73c 

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


Testing
---

Testing details are found at the end of this chain.


Thanks,

Greg Mann

Re: Review Request 53802: Eliminated an EOF race condition in libprocess SSL socket.

[Greg Mann]

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

(Updated Nov. 23, 2016, 12:58 a.m.)


Review request for mesos, Benjamin Mahler and Joseph Wu.


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


Repository: mesos


Description (updated)
---

Previously, it was possible for an SSL socket to either:
1) Fail to receive an EOF if the EOF event was received when
   there was no pending recv() request.
2) Fail to receive all data sent on the sending side if an
   EOF event was received before all sent data was read.

This patch eliminates these race conditions to ensure reliable
receipt of both sent data and EOFs.


Diffs (updated)
-

  3rdparty/libprocess/src/libevent_ssl_socket.hpp 
acb00d41c637a318b2f16fff9e97998b9c79b809 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
21f878ee81db32ad35878ec053c3f2de3637196c 

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


Testing
---

Testing details are found at the end of this chain.


Thanks,

Greg Mann

Re: Review Request 53802: Eliminated an EOF race condition in libprocess SSL socket.

[Greg Mann]

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

(Updated Nov. 23, 2016, 12:54 a.m.)


Review request for mesos, Benjamin Mahler and Joseph Wu.


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


Repository: mesos


Description (updated)
---

Previously, it was possible for an SSL socket to either:
1) Fail to receive an EOF if the EOF event was received when
   there was no pending recv() request.
2) Fail to receive all data sent on the sending side if an
   EOF event was received before all sent data was read.


This patch eliminates these race conditions to ensure reliable
receipt of both sent data and EOFs.


Diffs
-

  3rdparty/libprocess/src/libevent_ssl_socket.hpp 
acb00d41c637a318b2f16fff9e97998b9c79b809 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
21f878ee81db32ad35878ec053c3f2de3637196c 

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


Testing
---

Testing details are found at the end of this chain.


Thanks,

Greg Mann

Re: Review Request 53802: Eliminated an EOF race condition in libprocess SSL socket.

[Greg Mann]

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

(Updated Nov. 23, 2016, 12:52 a.m.)


Review request for mesos, Benjamin Mahler and Joseph Wu.


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


Repository: mesos


Description (updated)
---

Eliminated an EOF race condition in libprocess SSL socket.


Diffs (updated)
-

  3rdparty/libprocess/src/libevent_ssl_socket.hpp 
acb00d41c637a318b2f16fff9e97998b9c79b809 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
21f878ee81db32ad35878ec053c3f2de3637196c 

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


Testing
---

Testing details are found at the end of this chain.


Thanks,

Greg Mann

Re: Review Request 53802: Eliminated an EOF race condition in libprocess SSL socket.

[Joseph Wu]

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




3rdparty/libprocess/src/libevent_ssl_socket.cpp (lines 246 - 248)


We should probably amend this comment block with another section to explain 
the EOF bool.
Same with the last line of this diff.



3rdparty/libprocess/src/libevent_ssl_socket.cpp (lines 406 - 410)


I'm wondering if separating out this critical section into several chunks 
will introduce any bugs.

It may be worthwhile to move each of the three swaps into their associated 
`if-statement`.


- Joseph Wu


On Nov. 22, 2016, 11:55 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53802/
> ---
> 
> (Updated Nov. 22, 2016, 11:55 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Joseph Wu.
> 
> 
> Bugs: MESOS-5966
> https://issues.apache.org/jira/browse/MESOS-5966
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, it was possible for an SSL socket to either:
> 1) Fail to receive an EOF if the EOF event was received when
>there was no pending recv() request.
> 2) Fail to receive all data sent on the sending side if an
>EOF event was received before all sent data was read.
>   
> This patch eliminates these race conditions to ensure reliable
> receipt of both sent data and EOFs.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 
> acb00d41c637a318b2f16fff9e97998b9c79b809 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 21f878ee81db32ad35878ec053c3f2de3637196c 
> 
> Diff: https://reviews.apache.org/r/53802/diff/
> 
> 
> Testing
> ---
> 
> Testing details are found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 53802: Eliminated an EOF race condition in libprocess SSL socket.

[Greg Mann]

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

(Updated Nov. 22, 2016, 7:55 p.m.)


Review request for mesos, Benjamin Mahler and Joseph Wu.


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


Repository: mesos


Description (updated)
---

Previously, it was possible for an SSL socket to either:
1) Fail to receive an EOF if the EOF event was received when
   there was no pending recv() request.
2) Fail to receive all data sent on the sending side if an
   EOF event was received before all sent data was read.

This patch eliminates these race conditions to ensure reliable
receipt of both sent data and EOFs.


Diffs
-

  3rdparty/libprocess/src/libevent_ssl_socket.hpp 
acb00d41c637a318b2f16fff9e97998b9c79b809 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
21f878ee81db32ad35878ec053c3f2de3637196c 

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


Testing (updated)
---

Testing details are found at the end of this chain.


Thanks,

Greg Mann

Re: Review Request 53802: Eliminated an EOF race condition in libprocess SSL socket.

[Greg Mann]

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

(Updated Nov. 22, 2016, 7:51 p.m.)


Review request for mesos, Benjamin Mahler and Joseph Wu.


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


Repository: mesos


Description
---

Eliminated an EOF race condition in libprocess SSL socket.


Diffs
-

  3rdparty/libprocess/src/libevent_ssl_socket.hpp 
acb00d41c637a318b2f16fff9e97998b9c79b809 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
21f878ee81db32ad35878ec053c3f2de3637196c 

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


Testing (updated)
---

Previously, it was possible for an SSL socket to either:
1) Fail to receive an EOF if the EOF event was received when
   there was no pending recv() request.
2) Fail to receive all data sent on the sending side if an
   EOF event was received before all sent data was read.

This patch eliminates these race conditions to ensure reliable
receipt of both sent data and EOFs.


Thanks,

Greg Mann

Re: Review Request 53802: Eliminated an EOF race condition in libprocess SSL socket.

[Greg Mann]

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

(Updated Nov. 22, 2016, 6:54 a.m.)


Review request for mesos, Benjamin Mahler and Joseph Wu.


Changes
---

Updated a comment.


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


Repository: mesos


Description (updated)
---

Eliminated an EOF race condition in libprocess SSL socket.


Diffs (updated)
-

  3rdparty/libprocess/src/libevent_ssl_socket.hpp 
acb00d41c637a318b2f16fff9e97998b9c79b809 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
21f878ee81db32ad35878ec053c3f2de3637196c 

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


Testing
---

Testing details found at the end of this review chain.


Thanks,

Greg Mann

Re: Review Request 53802: Eliminated an EOF race condition in libprocess SSL socket.

[Greg Mann]

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

(Updated Nov. 17, 2016, 12:27 a.m.)


Review request for mesos, Benjamin Mahler and Joseph Wu.


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


Repository: mesos


Description (updated)
---

Previously, it was possible for an SSL socket to either:
1) Fail to receive an EOF if the EOF event was received when
   there was no pending recv() request.
2) Fail to receive all data sent on the sending side if an
   EOF event was received before all sent data was read.

This patch eliminates these race conditions to ensure reliable
receipt of both sent data and EOFs.


Diffs
-

  3rdparty/libprocess/src/libevent_ssl_socket.hpp 
acb00d41c637a318b2f16fff9e97998b9c79b809 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
21f878ee81db32ad35878ec053c3f2de3637196c 

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


Testing
---

Testing details found at the end of this review chain.


Thanks,

Greg Mann

Re: Review Request 53802: Eliminated an EOF race condition in libprocess SSL socket.

[Greg Mann]

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

(Updated Nov. 16, 2016, 7:08 p.m.)


Review request for mesos, Benjamin Mahler and Joseph Wu.


Changes
---

Updated dependency.


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


Repository: mesos


Description (updated)
---

Eliminated an EOF race condition in libprocess SSL socket.


Diffs (updated)
-

  3rdparty/libprocess/src/libevent_ssl_socket.hpp 
acb00d41c637a318b2f16fff9e97998b9c79b809 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
21f878ee81db32ad35878ec053c3f2de3637196c 

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


Testing
---

Testing details found at the end of this review chain.


Thanks,

Greg Mann

Review Request 53802: Eliminated an EOF race condition in libprocess SSL socket.

[Greg Mann]

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

Review request for mesos, Benjamin Mahler and Joseph Wu.


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


Repository: mesos


Description
---

Previously, it was possible for an SSL socket to either:
1) Fail to receive an EOF if the EOF event was received when
   there was no pending `recv()` request.
2) Fail to receive all data sent on the sending side if an
   EOF event was received before all sent data was read.

This patch eliminates these race conditions to ensure reliable
receipt of both sent data and EOFs.


Diffs
-

  3rdparty/libprocess/src/libevent_ssl_socket.hpp 
acb00d41c637a318b2f16fff9e97998b9c79b809 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
21f878ee81db32ad35878ec053c3f2de3637196c 

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


Testing
---

Testing details found at the end of this review chain.


Thanks,

Greg Mann