Re: Review Request 47362: Added a test for the SSL head-of-line blocking issue in MESOS-5340.

2016-06-09 Thread Benjamin Mahler 


> On May 16, 2016, 4:43 p.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/src/tests/ssl_tests.cpp, lines 730-731
> > 
> >
> > IIUC, this is an implementation detail, which may change in the future. 
> > Currently our code relies on the callback from lebevent, which is invoked 
> > then data is sent. We actually don't care whether the SSL handshake has 
> > completed, we *just* want that a pending socket does not prevent us from 
> > accepting new connections.
> > 
> > How about
> > ```
> >   // We initiate a connection on which we will not send
> >   // any data. This may prevent the socket from entering
> >   // ready state and hence the future signaled.
> > ```

Hm.. not sure I follow your thoughts here. Libevent is the implementation 
detail, SSL handshake is an implementation detail of SSL, but it's an important 
aspect of this test.

> we just want that a pending socket does not prevent us from accepting new 
> connections.

What does "pending" mean or "ready" state mean? Sockets in themselves do not 
have a notion of "pending" or "ready" states, these words are revealing the 
notion of SSL handshaking / downgrading. Since we're testing SSL sockets here, 
it's helpful to discuss the aspects of SSL that we're testing, no?


> On May 16, 2016, 4:43 p.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/src/tests/ssl_tests.cpp, lines 755-759
> > 
> >
> > Why do you construct the response manually instead of using `http::OK`?

The first is consistency with the rest of the tests here, but I did look at 
whether I could use http::OK. The wrinkle is that we have to directly send the 
bytes on the socket and so we have to use the encoder. The response encoder 
requires the corresponding request object, which we don't have here (I'd have 
to make this test more inconsistent with the rest).


- Benjamin


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


On May 13, 2016, 9:46 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47362/
> ---
> 
> (Updated May 13, 2016, 9:46 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, and Neil 
> Conway.
> 
> 
> Bugs: MESOS-5340
> https://issues.apache.org/jira/browse/MESOS-5340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test works by creating a connection on which no data is sent (the SSL 
> handshake does not complete, nor is the socket downgraded). After which, we 
> expect that an HTTP request should succeed.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> 4d237815a03828b915e821c3af78132e2915c610 
> 
> Diff: https://reviews.apache.org/r/47362/diff/
> 
> 
> Testing
> ---
> 
> This test fails before the fix in MESOS-5340.
> 
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from SSLTest
> [ RUN  ] SSLTest.SilentSocket
> ../../../3rdparty/libprocess/src/tests/ssl_tests.cpp:752: Failure
> Failed to wait 15secs for socket
> [  FAILED  ] SSLTest.SilentSocket (15221 ms)
> [--] 1 test from SSLTest (15221 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (15222 ms total)
> [  PASSED  ] 0 tests.
> [  FAILED  ] 1 test, listed below:
> [  FAILED  ] SSLTest.SilentSocket
> ```
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 47362: Added a test for the SSL head-of-line blocking issue in MESOS-5340.

2016-06-09 Thread Benjamin Mahler 


> On May 14, 2016, 3:59 a.m., haosdent huang wrote:
> > 3rdparty/libprocess/src/tests/ssl_tests.cpp, line 736
> > 
> >
> > Have a question here, why settle and resume without any operations here?

It was to ensure that the socket connection remains pending (the condition we 
check right after settling), it's technically not enough to reliably catch it 
(I'll document that).


> On May 14, 2016, 3:59 a.m., haosdent huang wrote:
> > 3rdparty/libprocess/src/tests/ssl_tests.cpp, line 755
> > 
> >
> > I think should change to
> > 
> > ```
> >   const string buffer =
> > string("HTTP/1.1 200 OK\r\n") +
> > "Content-Length : " +
> > stringify(data.length()) + "\r\n" +
> > "\r\n" +
> > data;
> > ```
> > 
> > here?

I originally had this but I found it less readable to indent the first line of 
the response due to the string construction.


- Benjamin


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


On May 13, 2016, 9:46 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47362/
> ---
> 
> (Updated May 13, 2016, 9:46 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, and Neil 
> Conway.
> 
> 
> Bugs: MESOS-5340
> https://issues.apache.org/jira/browse/MESOS-5340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test works by creating a connection on which no data is sent (the SSL 
> handshake does not complete, nor is the socket downgraded). After which, we 
> expect that an HTTP request should succeed.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> 4d237815a03828b915e821c3af78132e2915c610 
> 
> Diff: https://reviews.apache.org/r/47362/diff/
> 
> 
> Testing
> ---
> 
> This test fails before the fix in MESOS-5340.
> 
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from SSLTest
> [ RUN  ] SSLTest.SilentSocket
> ../../../3rdparty/libprocess/src/tests/ssl_tests.cpp:752: Failure
> Failed to wait 15secs for socket
> [  FAILED  ] SSLTest.SilentSocket (15221 ms)
> [--] 1 test from SSLTest (15221 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (15222 ms total)
> [  PASSED  ] 0 tests.
> [  FAILED  ] 1 test, listed below:
> [  FAILED  ] SSLTest.SilentSocket
> ```
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 47362: Added a test for the SSL head-of-line blocking issue in MESOS-5340.

2016-06-09 Thread Benjamin Mahler 


> On May 13, 2016, 9:59 p.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/src/tests/ssl_tests.cpp, line 27
> > 
> >
> > #include 

I didn't want to mix cleanups in this patch given that Try is already required, 
there are additional headers that are missing.

I'll follow up with a separate patch.


> On May 13, 2016, 9:59 p.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/src/tests/ssl_tests.cpp, line 754
> > 
> >
> > s/send(/send (/

I originally wanted to maintain consistency of comments these tests (this 
comment is used elsewhere), but I'll just rephrase it.


- Benjamin


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


On May 13, 2016, 9:46 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47362/
> ---
> 
> (Updated May 13, 2016, 9:46 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, and Neil 
> Conway.
> 
> 
> Bugs: MESOS-5340
> https://issues.apache.org/jira/browse/MESOS-5340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test works by creating a connection on which no data is sent (the SSL 
> handshake does not complete, nor is the socket downgraded). After which, we 
> expect that an HTTP request should succeed.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> 4d237815a03828b915e821c3af78132e2915c610 
> 
> Diff: https://reviews.apache.org/r/47362/diff/
> 
> 
> Testing
> ---
> 
> This test fails before the fix in MESOS-5340.
> 
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from SSLTest
> [ RUN  ] SSLTest.SilentSocket
> ../../../3rdparty/libprocess/src/tests/ssl_tests.cpp:752: Failure
> Failed to wait 15secs for socket
> [  FAILED  ] SSLTest.SilentSocket (15221 ms)
> [--] 1 test from SSLTest (15221 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (15222 ms total)
> [  PASSED  ] 0 tests.
> [  FAILED  ] 1 test, listed below:
> [  FAILED  ] SSLTest.SilentSocket
> ```
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 47362: Added a test for the SSL head-of-line blocking issue in MESOS-5340.

2016-05-16 Thread Alexander Rukletsov 

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




3rdparty/libprocess/src/tests/ssl_tests.cpp (lines 730 - 731)


IIUC, this is an implementation detail, which may change in the future. 
Currently our code relies on the callback from lebevent, which is invoked then 
data is sent. We actually don't care whether the SSL handshake has completed, 
we *just* want that a pending socket does not prevent us from accepting new 
connections.

How about
```
  // We initiate a connection on which we will not send
  // any data. This may prevent the socket from entering
  // ready state and hence the future signaled.
```



3rdparty/libprocess/src/tests/ssl_tests.cpp (line 744)


If you accept the suggestion above, it makes sense to kill this sentence.



3rdparty/libprocess/src/tests/ssl_tests.cpp (lines 755 - 759)


Why do you construct the response manually instead of using `http::OK`?


- Alexander Rukletsov


On May 13, 2016, 9:46 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47362/
> ---
> 
> (Updated May 13, 2016, 9:46 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, and Neil 
> Conway.
> 
> 
> Bugs: MESOS-5340
> https://issues.apache.org/jira/browse/MESOS-5340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test works by creating a connection on which no data is sent (the SSL 
> handshake does not complete, nor is the socket downgraded). After which, we 
> expect that an HTTP request should succeed.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> 4d237815a03828b915e821c3af78132e2915c610 
> 
> Diff: https://reviews.apache.org/r/47362/diff/
> 
> 
> Testing
> ---
> 
> This test fails before the fix in MESOS-5340.
> 
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from SSLTest
> [ RUN  ] SSLTest.SilentSocket
> ../../../3rdparty/libprocess/src/tests/ssl_tests.cpp:752: Failure
> Failed to wait 15secs for socket
> [  FAILED  ] SSLTest.SilentSocket (15221 ms)
> [--] 1 test from SSLTest (15221 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (15222 ms total)
> [  PASSED  ] 0 tests.
> [  FAILED  ] 1 test, listed below:
> [  FAILED  ] SSLTest.SilentSocket
> ```
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 47362: Added a test for the SSL head-of-line blocking issue in MESOS-5340.

2016-05-13 Thread haosdent huang 

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




3rdparty/libprocess/src/tests/ssl_tests.cpp (line 736)


Have a question here, why settle and resume without any operations here?



3rdparty/libprocess/src/tests/ssl_tests.cpp (line 755)


I think should change to

```
  const string buffer =
string("HTTP/1.1 200 OK\r\n") +
"Content-Length : " +
stringify(data.length()) + "\r\n" +
"\r\n" +
data;
```

here?


- haosdent huang


On May 13, 2016, 9:46 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47362/
> ---
> 
> (Updated May 13, 2016, 9:46 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, and Neil 
> Conway.
> 
> 
> Bugs: MESOS-5340
> https://issues.apache.org/jira/browse/MESOS-5340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test works by creating a connection on which no data is sent (the SSL 
> handshake does not complete, nor is the socket downgraded). After which, we 
> expect that an HTTP request should succeed.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> 4d237815a03828b915e821c3af78132e2915c610 
> 
> Diff: https://reviews.apache.org/r/47362/diff/
> 
> 
> Testing
> ---
> 
> This test fails before the fix in MESOS-5340.
> 
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from SSLTest
> [ RUN  ] SSLTest.SilentSocket
> ../../../3rdparty/libprocess/src/tests/ssl_tests.cpp:752: Failure
> Failed to wait 15secs for socket
> [  FAILED  ] SSLTest.SilentSocket (15221 ms)
> [--] 1 test from SSLTest (15221 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (15222 ms total)
> [  PASSED  ] 0 tests.
> [  FAILED  ] 1 test, listed below:
> [  FAILED  ] SSLTest.SilentSocket
> ```
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 47362: Added a test for the SSL head-of-line blocking issue in MESOS-5340.

2016-05-13 Thread Mesos ReviewBot 

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



Bad patch!

Reviews applied: [47362]

Failed command: ./support/apply-review.sh -n -r 47362

Error:
2016-05-14 02:14:21 URL:https://reviews.apache.org/r/47362/diff/raw/ 
[2478/2478] -> "47362.patch" [1]
Total errors found: 0
Checking 1 files
Error: No line in the commit message summary may exceed 72 characters.

Full log: https://builds.apache.org/job/mesos-reviewbot/13059/console

- Mesos ReviewBot


On May 13, 2016, 9:46 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47362/
> ---
> 
> (Updated May 13, 2016, 9:46 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, and Neil 
> Conway.
> 
> 
> Bugs: MESOS-5340
> https://issues.apache.org/jira/browse/MESOS-5340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test works by creating a connection on which no data is sent (the SSL 
> handshake does not complete, nor is the socket downgraded). After which, we 
> expect that an HTTP request should succeed.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> 4d237815a03828b915e821c3af78132e2915c610 
> 
> Diff: https://reviews.apache.org/r/47362/diff/
> 
> 
> Testing
> ---
> 
> This test fails before the fix in MESOS-5340.
> 
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from SSLTest
> [ RUN  ] SSLTest.SilentSocket
> ../../../3rdparty/libprocess/src/tests/ssl_tests.cpp:752: Failure
> Failed to wait 15secs for socket
> [  FAILED  ] SSLTest.SilentSocket (15221 ms)
> [--] 1 test from SSLTest (15221 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (15222 ms total)
> [  PASSED  ] 0 tests.
> [  FAILED  ] 1 test, listed below:
> [  FAILED  ] SSLTest.SilentSocket
> ```
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Review Request 47362: Added a test for the SSL head-of-line blocking issue in MESOS-5340.

2016-05-13 Thread Benjamin Mahler 

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

Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, and Neil 
Conway.


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


Repository: mesos


Description
---

This test works by creating a connection on which no data is sent (the SSL 
handshake does not complete, nor is the socket downgraded). After which, we 
expect that an HTTP request should succeed.


Diffs
-

  3rdparty/libprocess/src/tests/ssl_tests.cpp 
4d237815a03828b915e821c3af78132e2915c610 

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


Testing
---

This test fails before the fix in MESOS-5340.

```
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from SSLTest
[ RUN  ] SSLTest.SilentSocket
../../../3rdparty/libprocess/src/tests/ssl_tests.cpp:752: Failure
Failed to wait 15secs for socket
[  FAILED  ] SSLTest.SilentSocket (15221 ms)
[--] 1 test from SSLTest (15221 ms total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (15222 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] SSLTest.SilentSocket
```


Thanks,

Benjamin Mahler