You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Ilya Pronin <ip...@twopensource.com> on 2017/01/05 18:36:34 UTC
Re: Review Request 54873: Added asynchronous libcurl support to Mesos.
-----------------------------------------------------------
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
>
>