Replace entire cpprest usage with libcurl #66

shishirb-MSFT · 2021-09-02T00:22:58Z

No description provided.

This reverts commit 7d22e10.

shishirb-MSFT · 2021-09-02T00:54:19Z

client-lite/src/util/do_curl_multi_operation.h

+#include <curl/curl.h>
+#include "do_event.h"
+
+class CurlMultiOperation


CurlMultiOperation

I might do some refactoring and add comments in another PR. Let me know if you have any questions here. #Resolved

shishirb-MSFT · 2021-09-02T00:54:57Z

NOTICE.md


 ====================================================================

-microsoft/cpprestsdk 18212a2a7967e12d740bfb957e500892b3463c88 - MIT


cpprestsdk

I'll follow up with Carmen about adding libcurl info here and in cgmanifest.

jimson-msft · 2021-09-02T04:34:16Z

client-lite/src/download/download.cpp

 #include "do_common.h"
 #include "download.h"

+#include <cmath>


is this unused? #Closed

It is used for std::round on line 455. I think it was being brought in by one of cpprest's headers earlier.

jimson-msft · 2021-09-02T17:14:14Z

build/bootstrap.sh


    # Open-source library dependencies
-    # Boost libs for DO
+    # Boost libs


Can just delete this comment now. Before it was to distinguish which were used for DO and which were used for cpprest #Closed

jimson-msft · 2021-09-02T18:16:50Z

client-lite/src/util/http_agent.cpp

+    _requestContext.hrTranslatedStatusCode = S_OK;
+    _requestContext.responseOnHeadersAvailableInvoked = false;
+    _requestContext.responseOnCompleteInvoked = false;
+    _callbackContext = callerContext;


callerContext;

Do we need to expose this as a function param? Looks like it's never set and default value is always used. It looks like a param for the IHttpEvents interface but unused by the functions implementing that interface #WontFix

Will remove in the next PR.

jimson-msft · 2021-09-02T18:39:16Z

client-lite/src/util/http_agent.cpp

+        DoLogVerbose("New http_client for %s", szUrl);
+
+        // Set options that are generic to all requests
+        curl_easy_setopt(_requestContext.curlHandle, CURLOPT_FOLLOWLOCATION, static_cast<long>(1));


curl_easy_setopt(_requestContext.curlHandle, CURLOPT_FOLLOWLOCATION, static_cast(1));

Maybe I misunderstood but you mentioned in sync 302'ing urls needed manual handling? Doesn't this take care of redirects? #Resolved

This will make libcurl follow the redirect automatically but we still get headers back for the 302 response before getting headers for the 200.

jimson-msft · 2021-09-02T18:40:14Z

client-lite/src/util/http_agent.cpp

+
+        // Set options that are generic to all requests
+        curl_easy_setopt(_requestContext.curlHandle, CURLOPT_FOLLOWLOCATION, static_cast<long>(1));
+        curl_easy_setopt(_requestContext.curlHandle, CURLOPT_MAXREDIRS, static_cast<long>(10));


10

-1 allows infinite redericts, 10 to avoid infinite redirecting (can such a thing happen?) #Closed

Correct. It can happen so we limit it.

jimson-msft

shishirb-MSFT · 2021-09-02T22:00:34Z

client-lite/src/exe/docs.cpp


    ConfigManager clientConfigs;
-    auto downloadManager = std::make_shared<DownloadManager>(clientConfigs);
+    CurlMultiOperation curlOperations;


curlOperations

Move this to DownloadManager. #Resolved

jimson-msft · 2021-09-02T22:50:44Z

client-lite/src/util/do_curl_multi_operation.cpp

+    }
+}
+
+void CurlMultiOperation::_WorkerThread()


Consider renaming this to _DoWork() #Resolved

jimson-msft · 2021-09-02T22:59:47Z

client-lite/src/util/do_curl_multi_operation.cpp

+
+        if (_activeHandles.Empty())
+        {
+            _cv.wait(lock, [this](){ return !_fKeepRunning || !_handlesToAdd.empty(); });


!_handlesToAdd.empty()

if this is non-empty, wouldn't cv wait here forever? There's no way to drain/clear the vector except for line 89 right? for example if AddHandle() is called while this thread is executing between line 89 and 99 - wouldn't this e stuck unless fkeepRunning gets set to false? #Closed

It is the opposite - wait() will return if _handlesToAdd becomes non-empty or _fKeepRunning becomes false.

Also note that AddHandle will wait if we were executing line 89-99 at that time (same mutex).

jimson-msft · 2021-09-02T23:10:58Z

client-lite/src/util/do_curl_multi_operation.cpp

+                DO_ASSERT(false); // TODO(shishirb) how to handle?
+                break;
+
+            case 0:


I think you might have mentioned this when we went over this, but for case 0: it indicates the process timed out? Should a comment be added on why we leave this case empty? #Closed

0 = timeout, yes. Note this case isn't empty but falls through to call curl_multi_perform. I've added some comments in the next iteration.

* Agent now uses the curl lib with the multi interface with the select model of doing work. * Agent: Explicit link to pthread lib (cpprest was doing it on our behalf). * DownloadManagerTests: use polling, temporarily increase wait time * Add libcurl to bootstrap. Remove cpprest.

shishirb-MSFT added 15 commits September 1, 2021 00:59

Agent: Use curl in HttpAgent (incomplete)

b403e26

Agent: Remove cpprest. Explicit link to pthread.

408c791

Agent: Full libcurl usage compiling

97d18eb

Fix wrong wait predicate in curlops

a6109ba

Avoid extra CRLF in QueryHeaders output

e492b42

RemoveHandle need not remove from handlesToRemove

16ebdea

HttpAgent: Implement fail fast on errors

079a73c

Download: bytesTotal set to zero on failure

7d22e10

DownloadManagerTests: use polling, temporarily increase wait time

b035a60

Revert "Download: bytesTotal set to zero on failure"

0b4fddb

This reverts commit 7d22e10.

HttpAgent: Skip calling OnHeadersAvail for failures

474c5a2

Fix locking in CurlMultiOperation

650d93a

HttpAgent: Use curlResult in completion callback

28a61cd

HttpAgent: Fix redirects

c05df80

Add libcurl to bootstrap. Remove cpprest.

e167ae3

shishirb-MSFT requested a review from a team as a code owner September 2, 2021 00:22

shishirb-MSFT commented Sep 2, 2021

View reviewed changes

jimson-msft reviewed Sep 2, 2021

View reviewed changes

PR feedback

e9cec97

jimson-msft reviewed Sep 2, 2021

View reviewed changes

jimson-msft approved these changes Sep 2, 2021

View reviewed changes

shishirb-MSFT commented Sep 2, 2021

View reviewed changes

Move curlOps into DownloadManager

f033b24

jimson-msft reviewed Sep 2, 2021

View reviewed changes

Rename to _DoWork. Comments.

b23631a

shishirb-MSFT merged commit 70140de into feature/replace-cpprest Sep 7, 2021

shishirb-MSFT deleted the user/shishirb/ni-use-libcurl branch September 7, 2021 18:58


		====================================================================

		microsoft/cpprestsdk 18212a2a7967e12d740bfb957e500892b3463c88 - MIT

Replace entire cpprest usage with libcurl #66

Replace entire cpprest usage with libcurl #66

Uh oh!

Conversation

shishirb-MSFT commented Sep 2, 2021

Uh oh!

shishirb-MSFT Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shishirb-MSFT Sep 2, 2021

Choose a reason for hiding this comment

Uh oh!

jimson-msft Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shishirb-MSFT Sep 2, 2021

Choose a reason for hiding this comment

Uh oh!

jimson-msft Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimson-msft Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shishirb-MSFT Sep 2, 2021

Choose a reason for hiding this comment

Uh oh!

jimson-msft Sep 2, 2021 • edited by shishirb-MSFT Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shishirb-MSFT Sep 2, 2021

Choose a reason for hiding this comment

Uh oh!

jimson-msft Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shishirb-MSFT Sep 2, 2021

Choose a reason for hiding this comment

Uh oh!

jimson-msft left a comment

Choose a reason for hiding this comment

Uh oh!

shishirb-MSFT Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimson-msft Sep 2, 2021 • edited by shishirb-MSFT Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimson-msft Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shishirb-MSFT Sep 3, 2021

Choose a reason for hiding this comment

Uh oh!

jimson-msft Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shishirb-MSFT Sep 3, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

shishirb-MSFT Sep 2, 2021 •

edited

Loading

jimson-msft Sep 2, 2021 •

edited

Loading

jimson-msft Sep 2, 2021 •

edited

Loading

jimson-msft Sep 2, 2021 •

edited

Loading

jimson-msft Sep 2, 2021 •

edited by shishirb-MSFT

Loading

jimson-msft Sep 2, 2021 •

edited

Loading

shishirb-MSFT Sep 2, 2021 •

edited

Loading

jimson-msft Sep 2, 2021 •

edited by shishirb-MSFT

Loading

jimson-msft Sep 2, 2021 •

edited

Loading

jimson-msft Sep 2, 2021 •

edited

Loading