Skip to content

Commit c80a31f

Browse files
joka921joka921
authored
Work around a mean bug in boost:asio (#1238)
Define `BOOST_ASIO_DISABLE_AWAITABLE_FRAME_RECYCLING` to work around what seems to be a bug in `boost::asio` when different coroutine functions run concurrently.
1 parent 95fc20b commit c80a31f

File tree

2 files changed

+103
-84
lines changed

2 files changed

+103
-84
lines changed

‎src/util/http/beast.h‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,16 @@
3737
#define BOOST_BEAST_USE_STD_STRING_VIEW
3838
#endif
3939

40+
// Boost::ASIO currently seems to have a bug when multiple coroutine streams are
41+
// concurrently in flight because there were multiple calls to `co_spawn`.
42+
// `ASIO` recycles the memory for awaitable frames, but there seems to only be
43+
// one global unsynchronized memory pool for this recycling, which leads to all
44+
// kinds of race conditions. The following constant disables the memory
45+
// recycling and gets rid of all of those errors and crashes.
46+
// TODO<joka921> Further analyze and then report this bug to the ASIO
47+
// developers.
48+
#define BOOST_ASIO_DISABLE_AWAITABLE_FRAME_RECYCLING
49+
4050
#include <boost/asio.hpp>
4151
#include <boost/asio/ssl/stream.hpp>
4252
#include <boost/beast.hpp>

‎test/HttpTest.cpp‎

Lines changed: 93 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -17,96 +17,105 @@ using namespace ad_utility::httpUtils;
1717
using namespace boost::beast::http;
1818

1919
TEST(HttpServer, HttpTest) {
20-
// Create and run an HTTP server, which replies to each request with three
21-
// lines: the request method (GET, POST, or OTHER), a copy of the request
22-
// target (might be empty), and a copy of the request body (might be empty).
23-
TestHttpServer httpServer(
24-
[](auto request, auto&& send) -> boost::asio::awaitable<void> {
25-
std::string methodName;
26-
switch (request.method()) {
27-
case boost::beast::http::verb::get:
28-
methodName = "GET";
29-
break;
30-
case boost::beast::http::verb::post:
31-
methodName = "POST";
32-
break;
33-
default:
34-
methodName = "OTHER";
35-
}
36-
std::string response = absl::StrCat(
37-
methodName, "\n", toStd(request.target()), "\n", request.body());
38-
co_return co_await send(createOkResponse(
39-
response, request, ad_utility::MediaType::textPlain));
40-
});
41-
httpServer.runInOwnThread();
20+
// This test used to spuriously crash because of something that we (joka92,
21+
// RobinTF) currently consider to be a bug in Boost::ASIO. (See
22+
// `util/http/beast.h` for details). Repeat this test several times to make
23+
// such failures less spurious should they ever reoccur in the future.
24+
for (size_t k = 0; k < 10; ++k) {
25+
// Create and run an HTTP server, which replies to each request with three
26+
// lines: the request method (GET, POST, or OTHER), a copy of the request
27+
// target (might be empty), and a copy of the request body (might be empty).
28+
TestHttpServer httpServer(
29+
[](auto request, auto&& send) -> boost::asio::awaitable<void> {
30+
std::string methodName;
31+
switch (request.method()) {
32+
case boost::beast::http::verb::get:
33+
methodName = "GET";
34+
break;
35+
case boost::beast::http::verb::post:
36+
methodName = "POST";
37+
break;
38+
default:
39+
methodName = "OTHER";
40+
}
41+
std::string response = absl::StrCat(
42+
methodName, "\n", toStd(request.target()), "\n", request.body());
43+
co_return co_await send(createOkResponse(
44+
response, request, ad_utility::MediaType::textPlain));
45+
});
46+
httpServer.runInOwnThread();
4247

43-
// Create a client, and send a GET and a POST request in one session.
44-
// The constants in those test loops can be increased to find threading issues
45-
// using the thread sanitizer. However, these constants can't be higher by
46-
// default because the checks on GitHub actions will run forwever if they are.
47-
{
48-
std::vector<ad_utility::JThread> threads;
49-
for (size_t i = 0; i < 2; ++i) {
50-
threads.emplace_back([&]() {
51-
for (size_t j = 0; j < 5; ++j) {
52-
{
53-
HttpClient httpClient("localhost",
54-
std::to_string(httpServer.getPort()));
55-
ASSERT_EQ(
56-
httpClient.sendRequest(verb::get, "localhost", "target1").str(),
57-
"GET\ntarget1\n");
58-
ASSERT_EQ(
59-
httpClient
60-
.sendRequest(verb::post, "localhost", "target1", "body1")
61-
.str(),
62-
"POST\ntarget1\nbody1");
48+
// Create a client, and send a GET and a POST request in one session.
49+
// The constants in those test loops can be increased to find threading
50+
// issues using the thread sanitizer. However, these constants can't be
51+
// higher by default because the checks on GitHub actions will run forever
52+
// if they are.
53+
{
54+
std::vector<ad_utility::JThread> threads;
55+
for (size_t i = 0; i < 2; ++i) {
56+
threads.emplace_back([&]() {
57+
for (size_t j = 0; j < 5; ++j) {
58+
{
59+
HttpClient httpClient("localhost",
60+
std::to_string(httpServer.getPort()));
61+
ASSERT_EQ(
62+
httpClient.sendRequest(verb::get, "localhost", "target1")
63+
.str(),
64+
"GET\ntarget1\n");
65+
ASSERT_EQ(
66+
httpClient
67+
.sendRequest(verb::post, "localhost", "target1", "body1")
68+
.str(),
69+
"POST\ntarget1\nbody1");
70+
}
6371
}
64-
}
65-
});
72+
});
73+
}
6674
}
67-
}
6875

69-
// Do the same thing in a second session (to check if everything is still fine
70-
// with the server after we have communicated with it for one session).
71-
{
72-
HttpClient httpClient("localhost", std::to_string(httpServer.getPort()));
73-
ASSERT_EQ(httpClient.sendRequest(verb::get, "localhost", "target2").str(),
74-
"GET\ntarget2\n");
75-
ASSERT_EQ(
76-
httpClient.sendRequest(verb::post, "localhost", "target2", "body2")
77-
.str(),
78-
"POST\ntarget2\nbody2");
79-
}
76+
// Do the same thing in a second session (to check if everything is still
77+
// fine with the server after we have communicated with it for one session).
78+
{
79+
HttpClient httpClient("localhost", std::to_string(httpServer.getPort()));
80+
ASSERT_EQ(httpClient.sendRequest(verb::get, "localhost", "target2").str(),
81+
"GET\ntarget2\n");
82+
ASSERT_EQ(
83+
httpClient.sendRequest(verb::post, "localhost", "target2", "body2")
84+
.str(),
85+
"POST\ntarget2\nbody2");
86+
}
8087

81-
// Test if websocket is correctly opened and closed
82-
{
83-
HttpClient httpClient("localhost", std::to_string(httpServer.getPort()));
84-
auto response = httpClient.sendWebSocketHandshake(verb::get, "localhost",
85-
"/watch/some-id");
86-
// verify request is upgraded
87-
ASSERT_EQ(response.base().result(), http::status::switching_protocols);
88-
}
88+
// Test if websocket is correctly opened and closed
89+
{
90+
HttpClient httpClient("localhost", std::to_string(httpServer.getPort()));
91+
auto response = httpClient.sendWebSocketHandshake(verb::get, "localhost",
92+
"/watch/some-id");
93+
// verify request is upgraded
94+
ASSERT_EQ(response.base().result(), http::status::switching_protocols);
95+
}
8996

90-
// Test if websocket is denied on wrong paths
91-
{
92-
HttpClient httpClient("localhost", std::to_string(httpServer.getPort()));
93-
auto response = httpClient.sendWebSocketHandshake(verb::get, "localhost",
94-
"/other-path");
95-
// Check for not found error
96-
ASSERT_EQ(response.base().result(), http::status::not_found);
97-
}
97+
// Test if websocket is denied on wrong paths
98+
{
99+
HttpClient httpClient("localhost", std::to_string(httpServer.getPort()));
100+
auto response = httpClient.sendWebSocketHandshake(verb::get, "localhost",
101+
"/other-path");
102+
// Check for not found error
103+
ASSERT_EQ(response.base().result(), http::status::not_found);
104+
}
98105

99-
// Also test the convenience function `sendHttpOrHttpsRequest` (which creates
100-
// an own client for each request).
101-
{
102-
Url url{absl::StrCat("http://localhost:", httpServer.getPort(), "/target")};
103-
ASSERT_EQ(sendHttpOrHttpsRequest(url, verb::get).str(), "GET\n/target\n");
104-
ASSERT_EQ(sendHttpOrHttpsRequest(url, verb::post, "body").str(),
105-
"POST\n/target\nbody");
106-
}
106+
// Also test the convenience function `sendHttpOrHttpsRequest` (which
107+
// creates an own client for each request).
108+
{
109+
Url url{
110+
absl::StrCat("http://localhost:", httpServer.getPort(), "/target")};
111+
ASSERT_EQ(sendHttpOrHttpsRequest(url, verb::get).str(), "GET\n/target\n");
112+
ASSERT_EQ(sendHttpOrHttpsRequest(url, verb::post, "body").str(),
113+
"POST\n/target\nbody");
114+
}
107115

108-
// Check that after shutting down, no more new connections are accepted.
109-
httpServer.shutDown();
110-
ASSERT_ANY_THROW(
111-
HttpClient("localhost", std::to_string(httpServer.getPort())));
116+
// Check that after shutting down, no more new connections are accepted.
117+
httpServer.shutDown();
118+
ASSERT_ANY_THROW(
119+
HttpClient("localhost", std::to_string(httpServer.getPort())));
120+
}
112121
}

0 commit comments

Comments
 (0)