Page MenuHomePhabricator

[llvm] [Support] Add HTTP Client Support library.
ClosedPublic

Authored by noajshu on Oct 28 2021, 1:14 PM.

Details

Summary

This patch implements a small HTTP client library consisting primarily of the HTTPRequest, HTTPResponseHandler, and BufferedHTTPResponseHandler classes. Unit tests of the HTTPResponseHandler and BufferedHTTPResponseHandler are included.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
noajshu added inline comments.Nov 2 2021, 12:42 PM
llvm/lib/Support/HTTPClient.cpp
51

Ah, the perfect fit. Thanks!

llvm/unittests/Support/HTTPClient.cpp
158–163

I agree and I will remove the mockHTTPClientTest.

Do you think there is any value in the BrokenConnection test? I would be happy to remove this test as well. The only non-test code executed in that one is Client.perform(Request) and Client.get("") which is effectively a change-detector for the short convenience methods of the Client.

noajshu updated this revision to Diff 384247.Nov 2 2021, 2:55 PM

Update error handling of HTTPResponseHandler and HTTPClient

noajshu marked 5 inline comments as done and an inline comment as not done.Nov 2 2021, 3:58 PM
labath added a comment.Nov 3 2021, 2:15 AM

This looks good to me now, modulo some details in inline comments.

llvm/lib/Support/HTTPClient.cpp
30–32

= default;

llvm/unittests/Support/HTTPClient.cpp
18

This block looks weird -- I'd just make a separate test for that.

43–46

I don't see a need for that -- I can see how the caller might want to modify the buffer for its own purposes. and he can always convert it to a read-only buffer if he does not (the opposite is harder).

158–163

Yeah, I'd probably remove them, or at least significantly simplify them. If you want to test that the virtual perform is getting called, then all you need to do is mock that method -- you don't really need to provide a fake implementation of that method, and everything that gets called by it...

noajshu updated this revision to Diff 384612.Nov 3 2021, 4:58 PM
noajshu marked 4 inline comments as done.

Address final comments.

noajshu updated this revision to Diff 384633.Nov 3 2021, 6:53 PM
labath added a comment.Nov 5 2021, 7:35 AM

I think this is as good as it can get. What you do after that -- I don't know.

It wasn't clear to me whether you forgot about the mockHTTPClientBrokenConnectionTest part, or chose not to do that, but I added an additional comment to describe how I would do this.

llvm/lib/Support/HTTPClient.cpp
30

= default

llvm/unittests/Support/HTTPClient.cpp
141–142

MOCK_METHOD2(perform, Error(HTTPRequest, HTTPResponseHandler)), and then just set expectations in the test. The entire MockHTTPResponseHandler business can go away.

noajshu updated this revision to Diff 385186.Nov 5 2021, 2:08 PM

Make perform(Request, Handler) non-virtual, adopting D112753's convention for HTTPClient implementation.

noajshu updated this revision to Diff 385191.Nov 5 2021, 2:13 PM

nit, improve comment

noajshu updated this revision to Diff 385199.Nov 5 2021, 3:04 PM

Make HTTPClient::isAvailable static.

noajshu added a comment.EditedNov 5 2021, 3:17 PM

I think this is as good as it can get. What you do after that -- I don't know.

It wasn't clear to me whether you forgot about the mockHTTPClientBrokenConnectionTest part, or chose not to do that, but I added an additional comment to describe how I would do this.

Thanks Pavel! I decided to remove the mock client tests, because they only hit the few-line convenience methods. I'm happy to add them, I'm just unsure how to avoid making HTTPClient::perform(Request, Handler) virtual. In any case they will be hit in subsequent testing after a working client implementation (i.e. Curl in D112753) is added.

llvm/unittests/Support/HTTPClient.cpp
141–142

Thanks! It seems the only library code we hit with these tests is the small convenience methods perform(Request) and get(Url). Is it worth making the perform method virtual to be able to create the mock class?

noajshu updated this revision to Diff 385601.Nov 8 2021, 12:50 PM
noajshu marked 4 inline comments as done.

Final style updates.

noajshu updated this revision to Diff 385687.Nov 8 2021, 7:27 PM

Remove extraneous #define DEBUG_TYPE

labath added inline comments.Nov 9 2021, 4:26 AM
llvm/unittests/Support/HTTPClient.cpp
141–142

Probably not. It made sense while curl was implemented in a separate class, but I don't think it's worth going out of our way to make this work.

Couple of minor API design issues, but haven't dug in much.

@labath - what's your state on this and the subsequent patches? What would be best for me to focus on/do here to help out?

llvm/lib/Support/HTTPClient.cpp
26

Twine's mostly meant for when a string value may only be conditionally manifest (such as in a logging library where some dynamic log level would dictate whether the value is logged) - if the string is unconditionally manifest, Twine probably isn't the right tool to use here.

You could pass a SmallString by value & move it into the Url, but since it's a smallstring, it has a big sizeof and might not be the most efficient thing to move around. Alternatively just pass a StringRef.

(similar feedback up the stack/for other uses of Twine)

llvm/unittests/Support/HTTPClient.cpp
65–72

If this code isn't touching/implementing to_integer, it's probably not necessary/appropriate to re-test it here, should just rely on its existing definition/documentation/testing elsewhere.

86

Perhaps avoid the hardcoded 19 somehow? Like you could declare the string as a named char array, then use sizeof to get the length (-1 for the implicit null terminator, if that's not meant to be included)

noajshu updated this revision to Diff 386058.Nov 9 2021, 9:50 PM
noajshu marked 4 inline comments as done.

Respond to feedback from @dblaikie.

Thanks for the comments @dblaikie! I've converted to StringRef parameters throughout.

@labath - what's your state on this and the subsequent patches? What would be best for me to focus on/do here to help out?

I've pretty much done all I can with this series. I am happy with the code quality. Test coverage is ok. There's one ongoing discussion in https://reviews.llvm.org/D112753#inline-1083341, that I'd like to hear your thoughts on (if you have any), but that's a fairly small detail.

I'd say the main thing that's missing is for someone to say "this belongs in llvm".

noajshu updated this revision to Diff 386263.Nov 10 2021, 11:27 AM

Move global curl initialization / cleanup to static HTTPClient::initialize / HTTPClient::cleanup.

dblaikie accepted this revision.Nov 17 2021, 1:48 PM

@labath - what's your state on this and the subsequent patches? What would be best for me to focus on/do here to help out?

I've pretty much done all I can with this series. I am happy with the code quality. Test coverage is ok. There's one ongoing discussion in https://reviews.llvm.org/D112753#inline-1083341, that I'd like to hear your thoughts on (if you have any), but that's a fairly small detail.

I'd say the main thing that's missing is for someone to say "this belongs in llvm".

Fair enough - Yeah, I think the direction's worth having a go at - be nice to have it supported in llvm-symbolizer, etc.

llvm/include/llvm/Support/HTTPClient.h
32

Usually best to write op overloads as non-members (possibly as friends, if that's necessary/helpful) to avoid different conversion candidates an the LHS and RHS.

36

Skip const here

108

Skip top-level const here - while it's generally out of character for LLVM code, it's also not part of the function declaration (const here doesn't change the type of the function/doesn't change how people call it, it's only an implementation detail)

llvm/lib/Support/HTTPClient.cpp
26

Skip top-level "const" here - it's out of character for LLVM code.

75

skip const here

80–82

Use = default; here, perhaps?

86–90

Is there any use for this variable? It's currently only written and never read, by the looks of it. Maybe that's worth deferring until some use is implemented.

This revision is now accepted and ready to land.Nov 17 2021, 1:48 PM
noajshu updated this revision to Diff 388386.Nov 18 2021, 9:52 PM
noajshu marked 7 inline comments as done.

Remove unnecessary IsInitialized variable. Also remove some const qualifiers and replace empty ctor/dtor with =default

noajshu updated this revision to Diff 389341.Nov 23 2021, 3:46 PM

Small style/formatting update.

noajshu updated this revision to Diff 390202.Nov 28 2021, 8:50 AM

Add HTTPClient::setTimeout method.

dblaikie added inline comments.Nov 29 2021, 1:47 PM
llvm/unittests/Support/HTTPClient.cpp
76

This probably doesn't test what it's intended to test - the string will terminate at the first '\0' - if you want to test embedded null characters, you'd need to specify an explicit length when creating the StringRef explicitly, rather than relying on StringRef(const char*) conversion.

noajshu updated this revision to Diff 390465.Nov 29 2021, 2:12 PM
noajshu marked an inline comment as done.

String literal fix for unit test with embedded null characters.

llvm/unittests/Support/HTTPClient.cpp
76

Thanks for pointing this out! I have added a fix using C++14 string literals. If you prefer I can also change this to use a hardcoded string length instead.

dblaikie accepted this revision.Nov 29 2021, 3:47 PM
dblaikie added inline comments.
llvm/lib/Support/HTTPClient.cpp
74–75

Looks like this API doesn't require the HTTPResponseHandler to have a virtual dtor - if these objects will usually be owned directly, rather than indirectly through a unique_ptr of the base type, etc, then the dtor can be non-virtual (but the base type will have to make the dtor protected and concrete derived classes would need to be marked final)

llvm/unittests/Support/HTTPClient.cpp
76

Oh, great!

If the string data doesn't need to outlive the handleHeaderLine call, you could roll it in:

...handleHeaderLine("Content-Length: \0\0\0"s),
...

Rather than having a standalone named variable. Though if you prefer the named variable to document the code intent, that's totally fair too!

noajshu updated this revision to Diff 390771.Nov 30 2021, 11:43 AM
noajshu marked 2 inline comments as done.

Change HTTPRequestHandler dtor from virtual to protected.

Thanks for the tip on the protected dtor! I also changed the setTimeout to use std::chrono::milliseconds following the discussion on D112758.

noajshu updated this revision to Diff 390786.Nov 30 2021, 12:37 PM

remove extraneous semicolon

dblaikie accepted this revision.Nov 30 2021, 12:54 PM

Generally (still) looks good to me.

noajshu edited the summary of this revision. (Show Details)Dec 1 2021, 3:42 PM
This revision was automatically updated to reflect the committed changes.