----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54873/#review160119 -----------------------------------------------------------
src/curl/common.hpp (line 42) <https://reviews.apache.org/r/54873/#comment231123> s/Context/`Context` s/Session/`Session` s/session/`Session` We should replace all session with `Session`. src/curl/common.hpp (line 62) <https://reviews.apache.org/r/54873/#comment231132> These are just fly by comments. I was intrigued by the use of the Pimpl idiom and hence wanted to understand the implementation a bit better. src/curl/common.cpp (line 60) <https://reviews.apache.org/r/54873/#comment231130> Instead of `what` shouldn't this be `action`? `what` seems a bit confusing, since in the end this field is supposed to store the polling action that needs to be performed on this socket. src/curl/common.cpp (lines 222 - 225) <https://reviews.apache.org/r/54873/#comment231124> This is more of a question: Is it possible for `Context` to be invoked from two different `libprocess` threads. This would cause a lot of the assumptions to break here? Since `Context` is copyable and `ContextImpl` is a shared_ptr wondering if we need to think about a case where the `Context` might be invoked in two different libprocess threads. src/curl/common.cpp (line 241) <https://reviews.apache.org/r/54873/#comment231129> Why is this `else` condition only possible if the function is called from a timer callback. From the man page it looks like the "socketFunction" will be called by the "socketAction" whenever the status of the socket descriptor changes, so it might or might not be invoked from the timer callbacks? src/curl/common.cpp (lines 280 - 281) <https://reviews.apache.org/r/54873/#comment231125> Wasn't sure what you mean by a "race condition in discard"? Could you elaborate? src/curl/common.cpp (line 290) <https://reviews.apache.org/r/54873/#comment231133> We are invoking `socketAction` on `.onAny` so it could be invoked with a failed `Future` as well right? src/curl/common.cpp (line 314) <https://reviews.apache.org/r/54873/#comment231126> s/continut/continuing src/curl/common.cpp (line 332) <https://reviews.apache.org/r/54873/#comment231127> Why do we break here? Shouldn't we continue through the messages to see if any other "easy" handles are ready to be completed? src/curl/common.cpp (line 378) <https://reviews.apache.org/r/54873/#comment231131> What do you mean by a "different worker thread" ? Is this a libprocess thread ? I was under the impression that we shouldn't be using a context from different threads? - Avinash sridharan On Dec. 26, 2016, 1:41 a.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54873/ > ----------------------------------------------------------- > > (Updated Dec. 26, 2016, 1:41 a.m.) > > > Review request for mesos, Benjamin Hindman, Benjamin Mahler, Gilbert Song, > and Joris Van Remoortere. > > > Bugs: MESOS-4853 and MESOS-6129 > https://issues.apache.org/jira/browse/MESOS-4853 > https://issues.apache.org/jira/browse/MESOS-6129 > > > Repository: mesos > > > Description > ------- > > This patch integrates libcurl with Mesos's event runtime. It uses the > multi interface (https://curl.haxx.se/libcurl/c/libcurl-multi.html) to > avoid blocking operations. > > This patch introduces some common basic interfaces which all the > protocols (e.g., http/ftp/etc.) share. In the future, the plan is to > introduce more protocol specific interfaces (e.g., curl::http). > > > Diffs > ----- > > src/CMakeLists.txt c8d4260c03d8cdee1951a50d293e9fdabcd2cf84 > src/Makefile.am abcf7eed717ccd2396540011d7fb9fc7959c18f2 > src/curl/common.hpp PRE-CREATION > src/curl/common.cpp PRE-CREATION > src/tests/curl_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/54873/diff/ > > > Testing > ------- > > ``` > [==========] Running 6 tests from 1 test case. > > > [----------] Global test environment set-up. > [----------] 6 tests from CurlTest > [ RUN ] CurlTest.InvalidUrl > * Could not resolve host: invalidurl; Name or service not known > * Closing connection 0 > [ OK ] CurlTest.InvalidUrl (9 ms) > [ RUN ] CurlTest.SimpleUrl > * About to connect() to 10.0.49.2 port 42347 (#1) > * Trying 10.0.49.2... > * Connected to 10.0.49.2 (10.0.49.2) port 42347 (#1) > > GET /TestHttpServer/test HTTP/1.1 > Host: 10.0.49.2:42347 > Accept: */* > > < HTTP/1.1 200 OK > < Date: Sat, 24 Dec 2016 22:41:00 GMT > < Content-Length: 0 > < > * Connection #1 to host 10.0.49.2 left intact > [ OK ] CurlTest.SimpleUrl (4 ms) > [ RUN ] CurlTest.MultipleSessions > * About to connect() to 10.0.49.2 port 42347 (#2) > * Trying 10.0.49.2... > * Connected to 10.0.49.2 (10.0.49.2) port 42347 (#2) > > GET /TestHttpServer/test HTTP/1.1 > Host: 10.0.49.2:42347 > Accept: */* > > * Found bundle for host 10.0.49.2: 0x7f7f64017930 > * About to connect() to 10.0.49.2 port 42347 (#3) > * Trying 10.0.49.2... > * Connected to 10.0.49.2 (10.0.49.2) port 42347 (#3) > > GET /TestHttpServer/test HTTP/1.1 > Host: 10.0.49.2:42347 > Accept: */* > > < HTTP/1.1 200 OK > < Date: Sat, 24 Dec 2016 22:41:00 GMT > < Content-Length: 0 > < > * Connection #2 to host 10.0.49.2 left intact > < HTTP/1.1 202 Accepted > < Date: Sat, 24 Dec 2016 22:41:00 GMT > < Content-Length: 0 > < > * Connection #3 to host 10.0.49.2 left intact > [ OK ] CurlTest.MultipleSessions (4 ms) > [ RUN ] CurlTest.INTERNET_HttpsUrl > * About to connect() to registry-1.docker.io port 443 (#4) > * Trying 52.54.195.55... > * Connected to registry-1.docker.io (52.54.195.55) port 443 (#4) > * Initializing NSS with certpath: sql:/etc/pki/nssdb > * CAfile: /etc/pki/tls/certs/ca-bundle.crt > CApath: none > * SSL connection using TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 > * Server certificate: > * subject: CN=*.docker.io,OU=Domain Control Validated - > RapidSSL(R),OU=See www.rapidssl.com/resources/cps (c)15,OU=GT98568428 > * start date: Mar 19 17:34:32 2015 GMT > * expire date: Apr 21 01:51:52 2018 GMT > * common name: *.docker.io > * issuer: CN=RapidSSL SHA256 CA - G3,O=GeoTrust Inc.,C=US > > GET /v2/ HTTP/1.1 > Host: registry-1.docker.io > Accept: */* > > < HTTP/1.1 401 Unauthorized > < Content-Type: application/json; charset=utf-8 > < Docker-Distribution-Api-Version: registry/2.0 > < Www-Authenticate: Bearer > realm="https://auth.docker.io/token",service="registry.docker.io"; > < Date: Sat, 24 Dec 2016 22:41:01 GMT > < Content-Length: 87 > < Strict-Transport-Security: max-age=31536000 > < > {"errors":[{"code":"UNAUTHORIZED","message":"authentication > required","detail":null}]} > * Connection #4 to host registry-1.docker.io left intact > [ OK ] CurlTest.INTERNET_HttpsUrl (456 ms) > [ RUN ] CurlTest.Discard > * About to connect() to 10.0.49.2 port 42347 (#5) > * Trying 10.0.49.2... > * Connected to 10.0.49.2 (10.0.49.2) port 42347 (#5) > > GET /TestHttpServer/test HTTP/1.1 > Host: 10.0.49.2:42347 > Accept: */* > > * Closing connection 5 > [ OK ] CurlTest.Discard (5 ms) > [ RUN ] CurlTest.Timeout > * About to connect() to 10.0.49.2 port 42347 (#6) > * Trying 10.0.49.2... > * Connected to 10.0.49.2 (10.0.49.2) port 42347 (#6) > > GET /TestHttpServer/test HTTP/1.1 > Host: 10.0.49.2:42347 > Accept: */* > > * Operation timed out after 138 milliseconds with 0 out of -1 bytes received > * Closing connection 6 > [ OK ] CurlTest.Timeout (101 ms) > [----------] 6 tests from CurlTest (580 ms total) > > [----------] Global test environment tear-down > [==========] 6 tests from 1 test case ran. (636 ms total) > [ PASSED ] 6 tests. > ``` > > > Thanks, > > Jie Yu > >
