----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54873/#review160505 -----------------------------------------------------------
I got the feeling that nothing prevents `ContextImpl` completion handlers from executing concurrently in different worker threads. Also `ContextImpl::add()` can concurrently access the multi handle. Maybe `ContextImpl` should be made into a `Process` and have its handlers deferred through it? src/curl/common.cpp (lines 157 - 162) <https://reviews.apache.org/r/54873/#comment231668> I don't quite follow this part. You are referring to `ContextImpl::complete()` vs "onDiscard" callback race, right? Won't the inner callback (on line 162) be executed by any other free thread causing the same race? Does using the `Clock` somehow help here? src/curl/common.cpp (lines 241 - 247) <https://reviews.apache.org/r/54873/#comment231737> This part appears in the true-statement too. Shouldn't it be placed before `if`? src/curl/common.cpp (lines 430 - 448) <https://reviews.apache.org/r/54873/#comment231740> This transfers checking loop is the same as in `ContextImpl::socketAction()`. Maybe extract it into a separate method? src/curl/common.cpp (line 443) <https://reviews.apache.org/r/54873/#comment231738> A timer can potentially end more than 1 transfer. At least documentation doesn't say otherwise. I think it will be better to drop this `break`. src/curl/common.cpp (line 446) <https://reviews.apache.org/r/54873/#comment231739> Ditto as on line 443. - Ilya Pronin 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 > >
