Merge pull request pistacheio#1199 from dgreatwood/queuePopEntryFix

Queue pop-entry fix

include/pistache/mailbox.h

@@ -311,10 +311,47 @@ namespace Pistache
 
             if (isBound())
             {
-                uint64_t val;
+                uint64_t val = 0;
                 for (;;)
                 {
                     ssize_t bytes = read(event_fd, &val, sizeof val);
+
+                    // Note: Without the read-success check below, there can be
+                    // an issue that shows up in the test
+                    // multiple_client_with_requests_to_multithreaded_server
+                    // when calling run_http_server_test over and over again.
+                    //
+                    // Without the check, in a typical success case the read in
+                    // this function would be called twice. First time, read
+                    // reads val. Second time, read fails with errno = EAGAIN /
+                    // EWOULDBLOCK, causing us to break out of the loop and
+                    // return from our "pop" function here.
+                    // 
+                    // However, in the problem case, very occasionally two
+                    // pushes - and hence two writes to event_fd - occur just
+                    // ahead of the read, and both writes succeed by the time
+                    // we are calling read the second time in the for(;;)
+                    // loop. This causes the _second_ read to succeeed, which
+                    // clears the eventfd readiness caused by the write, even
+                    // though we have yet to pop that push off the queue. Hence
+                    // the values that were pushed might never be processed off
+                    // of this queue. So, we should break out of the loop when
+                    // the read succeeds.
+                    if (bytes == (sizeof val))
+                    { // success
+
+                        if (!ret)
+                        {
+                            // Have another try at pop, in case there was no
+                            // "pop" to do at the top of this function, but
+                            // then a push happened after the first pop attempt
+                            // but before the read above
+                            ret = Queue<T>::pop();
+                        }
+
+                        break;
+                    }
+
                     if (bytes == -1)
                     {
                         if (errno == EAGAIN || errno == EWOULDBLOCK)

tests/http_server_test.cc

@@ -345,6 +345,41 @@ TEST(http_server_test, multiple_client_with_requests_to_multithreaded_server)
     ASSERT_EQ(res2, SECOND_CLIENT_REQUEST_SIZE);
 }
 
+TEST(http_server_test, many_client_with_requests_to_multithreaded_server)
+{
+    const Pistache::Address address("localhost", Pistache::Port(0));
+
+    Http::Endpoint server(address);
+    auto flags       = Tcp::Options::ReuseAddr;
+    auto server_opts = Http::Endpoint::options().flags(flags).threads(6);
+    server.init(server_opts);
+    LOGGER("test", "Trying to run server...");
+    server.setHandler(Http::make_handler<HelloHandlerWithDelay>());
+    ASSERT_NO_THROW(server.serveThreaded());
+
+    const std::string server_address = "localhost:" + server.getPort().toString();
+    LOGGER("test", "Server address: " << server_address);
+
+    const int NO_TIMEOUT                = 0;
+    const int SECONDS_TIMOUT            = 10;
+    const int FIRST_CLIENT_REQUEST_SIZE = 128;
+    std::future<int> result1(std::async(clientLogicFunc,
+                                        FIRST_CLIENT_REQUEST_SIZE, server_address,
+                                        NO_TIMEOUT, SECONDS_TIMOUT));
+    const int SECOND_CLIENT_REQUEST_SIZE = 192;
+    std::future<int> result2(
+        std::async(clientLogicFunc, SECOND_CLIENT_REQUEST_SIZE, server_address,
+                   NO_TIMEOUT, SECONDS_TIMOUT));
+
+    int res1 = result1.get();
+    int res2 = result2.get();
+
+    server.shutdown();
+
+    ASSERT_EQ(res1, FIRST_CLIENT_REQUEST_SIZE);
+    ASSERT_EQ(res2, SECOND_CLIENT_REQUEST_SIZE);
+}
+
 TEST(http_server_test,
      multiple_client_with_different_requests_to_multithreaded_server)
 {
@@ -893,7 +928,8 @@ struct ContentEncodingHandler : public Http::Handler
         writer.setCompression(encoding);
 
         // Set compression level...
-        switch(encoding) {
+        switch (encoding)
+        {
 
 #ifdef PISTACHE_USE_CONTENT_ENCODING_BROTLI
         // Set maximum compression if using Brotli

version.txt

@@ -1 +1 @@
-0.2.7.20240223
+0.2.7.20240405