Merge bitcoin/bitcoin#36118: test: tolerate race condition in interface_http.py

a51df9b0ec test: tolerate race condition in interface_http.py (Matthew Zipkin)

Pull request description:

  Fixes #35632 by allowing both outcomes of a race condition. The server behavior is unchanged: in response to a malformed request we send an error code and disconnect. The issue is that sometimes on Windows the RST is caught by the platform and the receive buffer is discarded before the Python client can process it with recv().

  We can also be much more polite to misbehaving clients by implementing a lingering close using SO_LINGER as suggested in #35780 but that will require more review.

  The exact error in #35632 is hard to produce reliably but there are a few close options for reviewers. I tested this on windows native building with MSVC. In both of these cases the patch from this PR caught the error and passed the test.

  **RemoteDisconnected: Remote end closed connection without response**

  ```diff
  diff --git a/src/httpserver.cpp b/src/httpserver.cpp
  index 9bb89863af..62324d3fea 100644
  --- a/src/httpserver.cpp
  +++ b/src/httpserver.cpp
  @@ -1072,7 +1072,7 @@ std::unique_ptr<HTTPRequest> HTTPRemoteClient::TryReadRequest(const std::shared_
               e.what());

           // We failed to read a complete request from the buffer
  -        WriteNoStoreErrorReply(*client->m_req, HTTP_BAD_REQUEST);
  +        // WriteNoStoreErrorReply(*client->m_req, HTTP_BAD_REQUEST);
           client->m_disconnect = true;
           return nullptr;
       }
  ```

  **ConnectionResetError: [WinError 10054] An existing connection was forcibly closed by the remote host**

  ```diff
  diff --git a/src/httpserver.cpp b/src/httpserver.cpp
  index 9bb89863af..be52acb874 100644
  --- a/src/httpserver.cpp
  +++ b/src/httpserver.cpp
  @@ -1154,6 +1154,11 @@ bool HTTPRemoteClient::MaybeDisconnect(std::chrono::time_point<SteadyClock> now,
                "Disconnecting HTTP client %s (id=%llu)",
                m_origin,
                m_id);
  +    auto sock{GetSock()};
  +    linger opt{};
  +    opt.l_onoff  = 1;  // enable SO_LINGER
  +    opt.l_linger = 0;  // zero timeout
  +    sock->SetSockOpt(SOL_SOCKET, SO_LINGER, &opt, sizeof(opt));
       return true;
   }

  ```

ACKs for top commit:
  jeanpablojp:
    re-ACK a51df9b0ec
  winterrdog:
    tACK a51df9b0ec
  janb84:
    re ACK a51df9b0ec
  hodlinator:
    re-ACK a51df9b0ec
  sedited:
    ACK a51df9b0ec

Tree-SHA512: a6244581b2b51af647452e0dc8cd09cdc8d975dee6a0dc8b8064cad136023dad68b4af987303bced91a662bf5fae22871ea718a6a8e68024158a9aef6c5855ef

test/functional/interface_http.py

@@ -104,6 +104,26 @@ class HTTPBasicsTest (BitcoinTestFramework):
         self.setup_nodes()
         self.node = self.nodes[0]
 
+    def send_bad_and_tolerate_disconnect(self, conn, predicate_fn):
+        '''
+        Tolerate a race condition when sending a malformed request that should result
+        in the server disconnecting the client. The server *should* be sending an error
+        response as well but in some conditions on some platforms (Windows) the python
+        client might encounter the socket error before processing the response.
+        '''
+        with self.node.assert_debug_log([f"HTTPResponse (status code: {http.client.BAD_REQUEST}"]):
+            try:
+                response = predicate_fn()
+                assert_equal(response.status, http.client.BAD_REQUEST)
+                self.log.info(f"Client received expected {http.client.BAD_REQUEST} response before connection was terminated")
+                # Drain server response
+                response.read()
+                conn.set_timeout(2)
+            except NETWORK_ERRORS:
+                self.log.info(f"Client did not receive expected {http.client.BAD_REQUEST} response before connection was terminated")
+        assert conn.sock_closed()
+
+
     def run_test(self):
         # The test framework typically reuses a single persistent HTTP connection
         # for all RPCs to a TestNode. Because we are setting -rpcservertimeout
@@ -192,8 +212,7 @@ class HTTPBasicsTest (BitcoinTestFramework):
 
         # Excessive URI size plus default headers breaks the limit.
         conn = BitcoinHTTPConnection(self.node)
-        response2 = conn.get(f'/{"x" * MAX_HEADERS_SIZE}')
+        self.send_bad_and_tolerate_disconnect(conn, lambda: conn.get(f'/{"x" * MAX_HEADERS_SIZE}'))
-        assert_equal(response2.status, http.client.BAD_REQUEST)
 
         # Compute how many short header lines need to be added to http.client
         # default headers to make / break the total limit in a single request.
@@ -212,8 +231,7 @@ class HTTPBasicsTest (BitcoinTestFramework):
         conn = BitcoinHTTPConnection(self.node)
         for i in range(headers_above_limit):
             conn.add_header(f"header_{i:04}", "foo")
-        response3 = conn.get('/x')
+        self.send_bad_and_tolerate_disconnect(conn, lambda: conn.get('/x'))
-        assert_equal(response3.status, http.client.BAD_REQUEST)
 
         # Compute how much data we can add to a request message body
         # to make / break the limit.
@@ -593,8 +611,7 @@ class HTTPBasicsTest (BitcoinTestFramework):
         # Extra whitespace before colon in header.
         conn = BitcoinHTTPConnection(self.node)
         conn.headers = {"Authorization ": f"Basic {str_to_b64str(conn.authpair)}"}
-        response = conn.post('/', '{"method": "getbestblockhash"}')
+        self.send_bad_and_tolerate_disconnect(conn, lambda: conn.post('/', '{"method": "getbestblockhash"}'))
-        assert_equal(response.status, http.client.BAD_REQUEST)
 
         # Extra whitespace at start of new line.
         # "line folding" as defined in
@@ -603,8 +620,7 @@ class HTTPBasicsTest (BitcoinTestFramework):
         # https://www.rfc-editor.org/rfc/rfc7230#section-3.2.4
         conn = BitcoinHTTPConnection(self.node)
         conn.headers = {"Authorization": f"Basic \n {str_to_b64str(conn.authpair)}"}
-        response = conn.post('/', '{"method": "getbestblockhash"}')
+        self.send_bad_and_tolerate_disconnect(conn, lambda: conn.post('/', '{"method": "getbestblockhash"}'))
-        assert_equal(response.status, http.client.BAD_REQUEST)
 
 
     def check_connection_limit(self):