Code for communicating with another host via HTTP or HTTPS #823

hannahbast · 2022-11-12T21:59:09Z

Add a new class HttpClient which can open HTTP and HTTPS connections to a given host at a given port, and which can then send GET or POST requests, receive the response synchronously, and write it to a (potentially very large) string. Using Boost.Beast, just like for our HttpServer class.

Add a combined test for our HttpServer and HttpClient (for a meaningful test one needs a server and a client). So far, only tests the HTTP case because our HttpServer does not do HTTPS yet. But that is already much better than before when our HTTP communication is not tested at all.

Use the occasion to rename util/HttpServer (a directory with quite a few files in it) to util/http because httpServer was simply a misnomer. This became very apparent with the addition of the .h and .cpp file for HttpClient. This renaming of the directory required many small changes (in the includes and in the various CMakeLists.txt).

This will be needed in the PR for the support of the SERVICE clause, which comes when we are finished with this one.

hannahbast · 2022-11-12T22:09:54Z

@joka921 GitHub shows 47 changed files for this PR. However, it is really just six files where non-trivial stuff happens, the rest is due to renaming util/HttpServer to util/http. The six files, where something happens, are:

util/http/HttpClient.h      [completely new]
util/http/HttpClient.cpp    [completely new]
util/http/HttpUtils.cpp     [new function, rather simple] 
util/http/HttpServer.cpp    [small adaption, so that the infinite server loop can be broken if desired]
test/HttpTest.cpp           [new test, rather simple]
test/HttpUtilsTest.cpp      [new test, rather simple]

Add a new class HttpClient which can open HTTP and HTTPS connections to a given host, and which can then send GET or POST requests, receive the result synchronously, and write it to a (potentially very large) string. Uses Boost.Beast, just like our HttpServer. Add a combined test for our HttpServer and HttpClient (for a meaningful test one needs a server and a client). So far only tests the HTTP case because our HttpServer does not do HTTPS yet. Used the occasion to rename util/HttpServer to util/http because httpServer was simply a misnomer, which became even more apparent with the addition of HttpClient. This change required many small changes (in includes and in the various CMakeLists.txt).

joka921

This is already a great start.
I have some minor syntactical suggestions and very few questions that are worth discussion.

Currently some CI checks are failing:

You have to add OpenSSL-dev to the Dockerfile(s), this is done in your Service-PR but is also required here.
The failing of GCC12 seems odd to me. It is probably also related to our hacking of boost-asio. Can you try if it goes away if you include <utility> in our beast.h? That include has to appear before the include of the first Boost::Asio header.

src/util/BoostHelpers/AsyncWaitForFuture.h

src/util/http/ContentEncodingHelper.h

src/util/http/HttpClient.cpp

src/util/http/HttpServer.h

src/util/http/HttpClient.cpp

test/HttpTest.cpp

hannahbast · 2022-11-14T21:25:19Z

Note to self: rename header beast.h to AsioAndBeast.h or something like that.

Note: The seemingly large diff concerning `src/util/http/ContentEncodingHelper.h` stems from the fact that Robin stored that file (as well as several others) in DOS format. I changed it to UNIX format. This only affects the whitespace in the file.

joka921

I have two very minor suggestions,
And three not so minor (in relevance, not in size) requests concerning threadsafety.

src/util/http/HttpServer.h

src/util/http/HttpUtils.h

test/HttpTest.cpp

joka921 · 2022-11-15T08:56:24Z

test/HttpTest.cpp

-  httpServer.exitServerLoopAfterNextRequest();
-  HttpClient httpClient;
+  // Run the server in its own thread.
+  std::jthread httpServerThread([&]() { httpServer.run(); });


It is not neccesary neither correct to put the httpServer.run() call into a jthread

Necessary: The server internally creates the threads on which it actually runs (always at least two), it doesn't hijack the calling threads.

Incorrect: You assume, that the server.run() in the jthread completes before your first client sends a response. That are quite some assumptions on thread scheduling and the network stack that nobody guarantees to you.

TLDR:
A plain httpServer.run() inside the main thread should suffice and be correct.

I thought so, too, initially, and it was the very first thing I did when I wrote this test, but the httpServer.run() blocks.

My bad, that behavior is even described in detail in the run() function...
To make it correct, let the Server also have an atomic<bool> that is called isReady() or something which is set to true in the listener after the _acceptor.listen() has succeeded. Your test code can then wait for this bool to become true, and everything is safe.

src/util/http/beast.h

joka921

Answers to Hannah's questions concerning my previous review.

test/HttpTest.cpp

joka921 · 2022-11-15T18:42:55Z

test/HttpTest.cpp

-  httpServer.exitServerLoopAfterNextRequest();
-  HttpClient httpClient;
+  // Run the server in its own thread.
+  std::jthread httpServerThread([&]() { httpServer.run(); });


My bad, that behavior is even described in detail in the run() function...
To make it correct, let the Server also have an atomic<bool> that is called isReady() or something which is set to true in the listener after the _acceptor.listen() has succeeded. Your test code can then wait for this bool to become true, and everything is safe.

src/util/http/beast.h

Except waiting for the server to be ready in the next. I would prefer to do that together in a 1-1 session, since I am not sure what the best design here is.

Also made the tests safe for the case that the running of the server fails.

joka921

Small comments from my own changes.

src/util/http/HttpServer.h

joka921 · 2022-11-17T10:30:40Z

src/util/http/HttpServer.h

+  net::strand<net::io_context::executor_type> _acceptorStrand =
+      net::make_strand(_ioContext);
+  std::atomic<bool> _serverIsReady = false;
+  std::atomic<bool> _shutDownAfterNextConnectionIsAccepted = false;


Variable is now unused.

joka921 · 2022-11-17T10:32:20Z

src/util/http/HttpServer.h

-    // next connection right away).
+    // While the `_acceptor` is open, accept connections and handle each
+    // connection asynchronously (so that we are ready to accept the next
+    // connection right away). The `_acceptor` will be closed via the


Suggested change

// connection right away). The `_acceptor` will be closed via the

// connection right away). The `_acceptor` can be closed via the

joka921 · 2022-11-17T10:32:49Z

src/util/http/HttpServer.h

+    // While the `_acceptor` is open, accept connections and handle each
+    // connection asynchronously (so that we are ready to accept the next
+    // connection right away). The `_acceptor` will be closed via the
+    // `shutdown` method.


Suggested change

// `shutdown` method.

// `shutdown` method. This currently only happens in `test/HttpTest.cpp`

test/HttpTest.cpp

joka921

Thank you very much, I think we have converged to some version we both agree with.

hannahbast requested a review from joka921 November 12, 2022 21:59

hannahbast force-pushed the http-client branch from d6f4e57 to 0a221bf Compare November 12, 2022 22:58

hannahbast mentioned this pull request Nov 12, 2022

Basic support of SERVICE clause #793

Merged

joka921 requested changes Nov 14, 2022

View reviewed changes

joka921 requested changes Nov 15, 2022

View reviewed changes

joka921 reviewed Nov 15, 2022

View reviewed changes

Hannah Bast and others added 3 commits November 16, 2022 19:14

Address comments from Johannes second review

ad904cd

Except waiting for the server to be ready in the next. I would prefer to do that together in a 1-1 session, since I am not sure what the best design here is.

Waiting for the server to be up (somewhat hacky)

f9f23db

Made the shutdown mechanism a little cleaner.

6f930de

Also made the tests safe for the case that the running of the server fails.

joka921 reviewed Nov 17, 2022

View reviewed changes

joka921 and others added 3 commits November 17, 2022 11:38

Apply suggestions from code review

055863d

Clang format and removed an unused variable.

8e75478

Make httpServer a shared pointer + more detailed comments

b3cb13c

joka921 approved these changes Nov 17, 2022

View reviewed changes

hannahbast changed the title ~~Code for communicating with a client via HTTP or HTTPS~~ Code for communicating with another host via HTTP or HTTPS Nov 17, 2022

hannahbast changed the title ~~Code for communicating with another host via HTTP or HTTPS~~ Code for sending GET or POST requests to another host Nov 17, 2022

hannahbast changed the title ~~Code for sending GET or POST requests to another host~~ Code for communicating with another host via HTTP or HTTPS Nov 17, 2022

hannahbast merged commit 8d0026a into master Nov 17, 2022

hannahbast deleted the http-client branch November 17, 2022 20:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code for communicating with another host via HTTP or HTTPS #823

Code for communicating with another host via HTTP or HTTPS #823

hannahbast commented Nov 12, 2022

hannahbast commented Nov 12, 2022 •

edited

Loading

joka921 left a comment

hannahbast commented Nov 14, 2022

joka921 left a comment

joka921 Nov 15, 2022

hannahbast Nov 15, 2022

joka921 Nov 15, 2022

joka921 left a comment

joka921 Nov 15, 2022

joka921 left a comment

joka921 Nov 17, 2022

joka921 Nov 17, 2022

joka921 Nov 17, 2022

joka921 left a comment

	// connection right away). The `_acceptor` will be closed via the
	// connection right away). The `_acceptor` can be closed via the

	// `shutdown` method.
	// `shutdown` method. This currently only happens in `test/HttpTest.cpp`

Code for communicating with another host via HTTP or HTTPS #823

Code for communicating with another host via HTTP or HTTPS #823

Conversation

hannahbast commented Nov 12, 2022

hannahbast commented Nov 12, 2022 • edited Loading

joka921 left a comment

Choose a reason for hiding this comment

hannahbast commented Nov 14, 2022

joka921 left a comment

Choose a reason for hiding this comment

joka921 Nov 15, 2022

Choose a reason for hiding this comment

hannahbast Nov 15, 2022

Choose a reason for hiding this comment

joka921 Nov 15, 2022

Choose a reason for hiding this comment

joka921 left a comment

Choose a reason for hiding this comment

joka921 Nov 15, 2022

Choose a reason for hiding this comment

joka921 left a comment

Choose a reason for hiding this comment

joka921 Nov 17, 2022

Choose a reason for hiding this comment

joka921 Nov 17, 2022

Choose a reason for hiding this comment

joka921 Nov 17, 2022

Choose a reason for hiding this comment

joka921 left a comment

Choose a reason for hiding this comment

hannahbast commented Nov 12, 2022 •

edited

Loading