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

[Noah Shutty via Phabricator via llvm-commits]

noajshu added inline comments.


================
Comment at: llvm/include/llvm/Support/HTTPClient.h:83
+  void *Curl = nullptr;
+  Error curlInit();
+  ~HTTPClient();
----------------
labath wrote:
> 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.
Thank you for pointing out the issues with using `ManagedStatic`. I've moved the global init / deinit into static methods and modified `InitLLVM`'s ctor / dtor to call these.


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

https://reviews.llvm.org/D112753