[PATCH] D112753: [llvm] [Support] Add CURL HTTP Client.

[Pavel Labath via Phabricator via llvm-commits]

labath added inline comments.


================
Comment at: llvm/include/llvm/Support/HTTPClient.h:83
+  void *Curl = nullptr;
+  Error curlInit();
+  ~HTTPClient();
----------------
noajshu wrote:
> noajshu wrote:
> > labath wrote:
> > > noajshu wrote:
> > > > Hi @labath, do you think we should follow the [[ https://llvm.org/docs/ProgrammersManual.html#fallible-constructors | style guide recommendation ]] for fallible constructors instead of using `curlInit`? That is, `HTTPClient` would have a private constructor and a `static Expected<HTTPClient> Create()`, which would just run the code inside `curlInit`.
> > > Umm... maybe?
> > > 
> > > I don't think that doing this would be a bad choice, but for me, the more interesting question is whether we should rely on the implicit `curl_global_init` call that happens as a part of this. This seems to be discouraged by curl, and this kind of initialization is not a particularly nice thing for a library (which llvm is) to do.
> > > 
> > > If we can guarantee that `curl_global_init` was called, then we might not even need a fallible constructor here. The man page is not particularly clear on this, but I would expect that the only reason this can fail in this case is memory (or some other soft resource) exhaustion, and llvm is not robust against those anyway...
> > Thank you so much for pointing out this issue.
> > 
> > Three solutions come to mind:
> > 
> > 1. The elegant C++ solution:
> > ```
> > // HTTPClient.cpp
> > class CurlGlobalState {
> > public:
> >   CurlGlobalState() {
> >     curl_global_init(CURL_GLOBAL_ALL);
> >   }
> >   ~CurlGlobalState() {
> >     curl_global_cleanup(CURL_GLOBAL_ALL);
> >   }
> > };
> > static const CurlGlobalState CurlState;
> > ```
> > The only potential issue here is that a library user loading LLVM Support as a Windows DLL could hit a deadlock (noted [[ https://curl.se/libcurl/c/libcurl.html | here ]], more details [[ https://docs.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-best-practices | here ]]).
> > 
> > 2. The less-elegant solution: exposing the global init and cleanup functions to the user, e.g. wrapped by:
> > ```
> > static HTTPClient::globalInit();
> > static HTTPClient::globalCleanup();
> > ```
> > and just asking users to call these at the beginning and end of the program.
> > 
> > 3. The LLVM-ey compromise:
> > Throw the global init and cleanup into `InitLLVM` which [[ https://reviews.llvm.org/D45602 | seems to have been made ]] for purposes just like this.
> > 
> > Do you lean towards any particular solution?
> After some more looking, I think the [[ https://llvm.org/doxygen/classllvm_1_1ManagedStatic.html | ManagedStatic ]] template may be just what we want. I'm running some experiments to see if it will fit our needs here.
ManagedStatic would get around the ban on global constructors, but delaying the initialization until the first use means that the this could race with other calls to curl_global_init. If all uses go through this code, then it would be fine, but there's no way of knowing what will other libraries linked to the main application be doing.

This wouldn't be the first nor the worst case of llvm doing something non-library-friendly (I'm thinking of signal handlers here). However, it is not a practice I support, so I would prefer a solution (like InitLLVM) which lets the main application choose when and how the initialization happens.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112753/new/

https://reviews.llvm.org/D112753