From 0ff9a99aea2ede723673f8b658fd4bc8548f8d94 Mon Sep 17 00:00:00 2001 From: Tyler Karaszewski Date: Tue, 10 Oct 2023 10:21:04 -0700 Subject: [PATCH 1/4] A bunch of test changes that won't pass on travis --- BedrockServer.cpp | 2 +- test/clustertest/testplugin/TestPlugin.cpp | 10 ++++++++++ test/clustertest/tests/HTTPSTest.cpp | 23 ++++++++++++++++++++-- 3 files changed, 32 insertions(+), 3 deletions(-) diff --git a/BedrockServer.cpp b/BedrockServer.cpp index b79297266..c8c972e9b 100644 --- a/BedrockServer.cpp +++ b/BedrockServer.cpp @@ -671,7 +671,7 @@ void BedrockServer::worker(int threadId) }); // Get the next one. - command = commandQueue.get(1000000, !threadId); + command = commandQueue.get(1000000); SAUTOPREFIX(command->request); SINFO("Dequeued command " << command->request.methodLine << " (" << command->id << ") in worker, " diff --git a/test/clustertest/testplugin/TestPlugin.cpp b/test/clustertest/testplugin/TestPlugin.cpp index 3a8144596..a81a9ec60 100644 --- a/test/clustertest/testplugin/TestPlugin.cpp +++ b/test/clustertest/testplugin/TestPlugin.cpp @@ -84,6 +84,7 @@ unique_ptr BedrockPlugin_TestPlugin::getCommand(SQLiteCommand&& "sendrequest", "slowquery", "httpstimeout", + "httpsdelay", "exceptioninpeek", "generatesegfaultpeek", "generateassertpeek", @@ -275,6 +276,12 @@ bool TestPluginCommand::peek(SQLite& db) { transaction->s->send(newRequest.serialize()); }).detach(); } + } else if (SStartsWith(request.methodLine, "httpsdelay")) { + SINFO("Sending HTTPS request"); + SData newRequest("GET /delay.php HTTP/1.1"); + newRequest["Host"] = "www.expensify.com.dev"; + newRequest["Delay-Sec"] = request["Delay-Sec"]; + httpsRequests.push_back(plugin().httpsManager->send("https://www.expensify.com.dev/", newRequest)); } else if (SStartsWith(request.methodLine, "exceptioninpeek")) { throw 1; } else if (SStartsWith(request.methodLine, "generatesegfaultpeek")) { @@ -439,6 +446,9 @@ void TestPluginCommand::process(SQLite& db) { // Done. return; + } else if (SStartsWith(request.methodLine, "httpsdelay")) { + SINFO("Saving HTTPS response."); + response.content = httpsRequests.front()->fullResponse.content; } else if (SStartsWith(request.methodLine, "idcollision")) { usleep(1001); // for TimingTest to not get 0 values. SQResult result; diff --git a/test/clustertest/tests/HTTPSTest.cpp b/test/clustertest/tests/HTTPSTest.cpp index b35fcece9..65278a112 100644 --- a/test/clustertest/tests/HTTPSTest.cpp +++ b/test/clustertest/tests/HTTPSTest.cpp @@ -21,8 +21,9 @@ struct HTTPSTest : tpunit::TestFixture { : tpunit::TestFixture("HTTPS", BEFORE_CLASS(HTTPSTest::setup), AFTER_CLASS(HTTPSTest::teardown), - TEST(HTTPSTest::testMultipleRequests), - TEST(HTTPSTest::test)) { } + /*TEST(HTTPSTest::testMultipleRequests),*/ + TEST(HTTPSTest::testBlockingShutdown)/*, + TEST(HTTPSTest::test)*/) { } BedrockClusterTester* tester; @@ -105,4 +106,22 @@ struct HTTPSTest : tpunit::TestFixture { ASSERT_EQUAL(SToInt(code), 200); } } + + void testBlockingShutdown() { + size_t delaySec = 3; + thread t([this, delaySec]() { + BedrockTester& brtester = tester->getTester(0); + SData request("httpsdelay"); + request["Delay-Sec"] = to_string(delaySec); + auto result = brtester.executeWaitMultipleData({request})[0]; + cout << result.methodLine << endl; + }); + + // See how long it takes to shut down the server. + usleep(500'000); + auto start = STimeNow(); + tester->getTester(0).stopServer(); + cout << "Stopped server in " << (STimeNow() - start) << "us. HTTPS request was delayed by " << delaySec << " seconds." << endl; + t.join(); + } } __HTTPSTest; From 02e4cca9bf08b35dbf2167c00260c4fa8f9f39b6 Mon Sep 17 00:00:00 2001 From: Tyler Karaszewski Date: Tue, 10 Oct 2023 10:27:38 -0700 Subject: [PATCH 2/4] Pretty much final testing methodology --- test/clustertest/tests/HTTPSTest.cpp | 32 ++++++++++++++++------------ 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/test/clustertest/tests/HTTPSTest.cpp b/test/clustertest/tests/HTTPSTest.cpp index 65278a112..bc30726c9 100644 --- a/test/clustertest/tests/HTTPSTest.cpp +++ b/test/clustertest/tests/HTTPSTest.cpp @@ -108,20 +108,24 @@ struct HTTPSTest : tpunit::TestFixture { } void testBlockingShutdown() { - size_t delaySec = 3; - thread t([this, delaySec]() { - BedrockTester& brtester = tester->getTester(0); - SData request("httpsdelay"); - request["Delay-Sec"] = to_string(delaySec); - auto result = brtester.executeWaitMultipleData({request})[0]; - cout << result.methodLine << endl; - }); + for (auto delaySec : {0,5,10,20,30}) { + thread t([this, delaySec]() { + BedrockTester& brtester = tester->getTester(0); + SData request("httpsdelay"); + request["Delay-Sec"] = to_string(delaySec); + auto result = brtester.executeWaitMultipleData({request})[0]; + cout << "PHP responded: " << result.content << endl; + }); - // See how long it takes to shut down the server. - usleep(500'000); - auto start = STimeNow(); - tester->getTester(0).stopServer(); - cout << "Stopped server in " << (STimeNow() - start) << "us. HTTPS request was delayed by " << delaySec << " seconds." << endl; - t.join(); + // See how long it takes to shut down the server. + usleep(500'000); + auto start = STimeNow(); + tester->getTester(0).stopServer(); + cout << "Stopped server in " << ((STimeNow() - start) / 1'000'000) << " seconds. HTTPS request was delayed by " << delaySec << " seconds." << endl; + t.join(); + tester->getTester(0).startServer(); + tester->getTester(0).waitForState("LEADING"); + cout << "Server is leading again." << endl; + } } } __HTTPSTest; From 3af73ceb3ab06153bf73a3e0489fc1da514bf0c6 Mon Sep 17 00:00:00 2001 From: Tyler Karaszewski Date: Tue, 10 Oct 2023 10:53:31 -0700 Subject: [PATCH 3/4] Last test change --- BedrockServer.cpp | 25 +++++++++------------- test/clustertest/testplugin/TestPlugin.cpp | 2 +- test/clustertest/tests/HTTPSTest.cpp | 4 ++-- 3 files changed, 13 insertions(+), 18 deletions(-) diff --git a/BedrockServer.cpp b/BedrockServer.cpp index c8c972e9b..0f4e1e626 100644 --- a/BedrockServer.cpp +++ b/BedrockServer.cpp @@ -671,7 +671,7 @@ void BedrockServer::worker(int threadId) }); // Get the next one. - command = commandQueue.get(1000000); + command = commandQueue.get(1000000, !threadId); SAUTOPREFIX(command->request); SINFO("Dequeued command " << command->request.methodLine << " (" << command->id << ") in worker, " @@ -833,29 +833,24 @@ void BedrockServer::runCommand(unique_ptr&& _command, bool isBlo break; } - // There are two other special cases for the time to wait. - if (fdm.empty()) { - // If there are no sockets to poll, wait 1 second. - // Why would there be no sockets? It's because Auth::Stripe, as a rate-limiting feature, attaches sockets to requests after their made. - // This means a request can sit around with no actual socket attached to it for some length of time until it's turn to talk to Stripe comes up. - // If that happens though, and we're sitting in `poll` when it becomes our turn, we will wait the full five minute timeout of the original `poll` - // call before we time out and try again wit the newly-attached socket. - // Setting this to one second lets us try again more frequently. This is probably not the ideal way to handle this, but it works for now. - maxWaitUs = 1'000'000; - } - - // Also, if we're shutting down or standing down, wait 1 second. This keeps the rest of the server from being blocked on commands that won't finish. + // We never wait more than 1 second in `poll`. There are two uses for this. One is that at shutdown, we want to kill any sockets that have are making no progress. + // We don't want these to be stuck sitting for 5 minutes doing nothing while thew server hangs, so we will interrupt every second to check on them. + // The other case is that there can be no sockets at all. + // Why would there be no sockets? It's because Auth::Stripe, as a rate-limiting feature, attaches sockets to requests after their made. + // This means a request can sit around with no actual socket attached to it for some length of time until it's turn to talk to Stripe comes up. + // If that happens though, and we're sitting in `poll` when it becomes our turn, we will wait the full five minute timeout of the original `poll` + // call before we time out and try again wit the newly-attached socket. + // Setting this to one second lets us try again more frequently. + maxWaitUs = min(maxWaitUs, 1'000'000ul); bool shuttingDown = false; auto _syncNodeCopy = atomic_load(&_syncNode); if (_shutdownState.load() != RUNNING || (_syncNodeCopy && _syncNodeCopy->getState() == SQLiteNodeState::STANDINGDOWN)) { - maxWaitUs = 1'000'000; shuttingDown = true; } // Ok, go ahead and `poll`. S_poll(fdm, maxWaitUs); - // The 3rd parameter to `postPoll` here is the total allowed idle time on this connection. We will kill connections that do nothing at all after 5 minutes normally, // or after only 5 seconds when we're shutting down so that we can clean up and move along. uint64_t ignore{0}; diff --git a/test/clustertest/testplugin/TestPlugin.cpp b/test/clustertest/testplugin/TestPlugin.cpp index a81a9ec60..f302a5b6f 100644 --- a/test/clustertest/testplugin/TestPlugin.cpp +++ b/test/clustertest/testplugin/TestPlugin.cpp @@ -448,7 +448,7 @@ void TestPluginCommand::process(SQLite& db) { return; } else if (SStartsWith(request.methodLine, "httpsdelay")) { SINFO("Saving HTTPS response."); - response.content = httpsRequests.front()->fullResponse.content; + response.content = "HTTPS Manager response code: " + to_string(httpsRequests.front()->response); } else if (SStartsWith(request.methodLine, "idcollision")) { usleep(1001); // for TimingTest to not get 0 values. SQResult result; diff --git a/test/clustertest/tests/HTTPSTest.cpp b/test/clustertest/tests/HTTPSTest.cpp index bc30726c9..6f276e55e 100644 --- a/test/clustertest/tests/HTTPSTest.cpp +++ b/test/clustertest/tests/HTTPSTest.cpp @@ -108,13 +108,13 @@ struct HTTPSTest : tpunit::TestFixture { } void testBlockingShutdown() { - for (auto delaySec : {0,5,10,20,30}) { + for (auto delaySec : {0,3,5,10,20,30}) { thread t([this, delaySec]() { BedrockTester& brtester = tester->getTester(0); SData request("httpsdelay"); request["Delay-Sec"] = to_string(delaySec); auto result = brtester.executeWaitMultipleData({request})[0]; - cout << "PHP responded: " << result.content << endl; + cout << result.content << endl; }); // See how long it takes to shut down the server. From f5970327881352335448c4019f0f040afd00283d Mon Sep 17 00:00:00 2001 From: Tyler Karaszewski Date: Tue, 10 Oct 2023 10:53:53 -0700 Subject: [PATCH 4/4] Undo test changes --- test/clustertest/testplugin/TestPlugin.cpp | 10 -------- test/clustertest/tests/HTTPSTest.cpp | 27 ++-------------------- 2 files changed, 2 insertions(+), 35 deletions(-) diff --git a/test/clustertest/testplugin/TestPlugin.cpp b/test/clustertest/testplugin/TestPlugin.cpp index f302a5b6f..3a8144596 100644 --- a/test/clustertest/testplugin/TestPlugin.cpp +++ b/test/clustertest/testplugin/TestPlugin.cpp @@ -84,7 +84,6 @@ unique_ptr BedrockPlugin_TestPlugin::getCommand(SQLiteCommand&& "sendrequest", "slowquery", "httpstimeout", - "httpsdelay", "exceptioninpeek", "generatesegfaultpeek", "generateassertpeek", @@ -276,12 +275,6 @@ bool TestPluginCommand::peek(SQLite& db) { transaction->s->send(newRequest.serialize()); }).detach(); } - } else if (SStartsWith(request.methodLine, "httpsdelay")) { - SINFO("Sending HTTPS request"); - SData newRequest("GET /delay.php HTTP/1.1"); - newRequest["Host"] = "www.expensify.com.dev"; - newRequest["Delay-Sec"] = request["Delay-Sec"]; - httpsRequests.push_back(plugin().httpsManager->send("https://www.expensify.com.dev/", newRequest)); } else if (SStartsWith(request.methodLine, "exceptioninpeek")) { throw 1; } else if (SStartsWith(request.methodLine, "generatesegfaultpeek")) { @@ -446,9 +439,6 @@ void TestPluginCommand::process(SQLite& db) { // Done. return; - } else if (SStartsWith(request.methodLine, "httpsdelay")) { - SINFO("Saving HTTPS response."); - response.content = "HTTPS Manager response code: " + to_string(httpsRequests.front()->response); } else if (SStartsWith(request.methodLine, "idcollision")) { usleep(1001); // for TimingTest to not get 0 values. SQResult result; diff --git a/test/clustertest/tests/HTTPSTest.cpp b/test/clustertest/tests/HTTPSTest.cpp index 6f276e55e..b35fcece9 100644 --- a/test/clustertest/tests/HTTPSTest.cpp +++ b/test/clustertest/tests/HTTPSTest.cpp @@ -21,9 +21,8 @@ struct HTTPSTest : tpunit::TestFixture { : tpunit::TestFixture("HTTPS", BEFORE_CLASS(HTTPSTest::setup), AFTER_CLASS(HTTPSTest::teardown), - /*TEST(HTTPSTest::testMultipleRequests),*/ - TEST(HTTPSTest::testBlockingShutdown)/*, - TEST(HTTPSTest::test)*/) { } + TEST(HTTPSTest::testMultipleRequests), + TEST(HTTPSTest::test)) { } BedrockClusterTester* tester; @@ -106,26 +105,4 @@ struct HTTPSTest : tpunit::TestFixture { ASSERT_EQUAL(SToInt(code), 200); } } - - void testBlockingShutdown() { - for (auto delaySec : {0,3,5,10,20,30}) { - thread t([this, delaySec]() { - BedrockTester& brtester = tester->getTester(0); - SData request("httpsdelay"); - request["Delay-Sec"] = to_string(delaySec); - auto result = brtester.executeWaitMultipleData({request})[0]; - cout << result.content << endl; - }); - - // See how long it takes to shut down the server. - usleep(500'000); - auto start = STimeNow(); - tester->getTester(0).stopServer(); - cout << "Stopped server in " << ((STimeNow() - start) / 1'000'000) << " seconds. HTTPS request was delayed by " << delaySec << " seconds." << endl; - t.join(); - tester->getTester(0).startServer(); - tester->getTester(0).waitForState("LEADING"); - cout << "Server is leading again." << endl; - } - } } __HTTPSTest;