CelestiaTheDryad opened a new pull request, #516: URL: https://github.com/apache/httpcomponents-core/pull/516
We encountered issues where our client would send a request to the HTTP server and not receive a response, errored or otherwise. The request would remain in limbo until either side declared a socket timeout. The server application would receive the request and process it and submit the response to the Apache HTTP server. Wireshark showed the the HTTP server would ACK the request packet but never send data of its own. I eventually traced this to the server applying pipelining logic at https://github.com/apache/httpcomponents-core/blob/d0707cdca67dc4084e372c04a675cda132e20649/httpcore5/src/main/java/org/apache/hc/core5/http/impl/nio/ServerHttp1StreamDuplexer.java#L368-L378 This was interesting, as the client was not pipelining requests. I determined this was because the server was skipping https://github.com/apache/httpcomponents-core/blob/d0707cdca67dc4084e372c04a675cda132e20649/httpcore5/src/main/java/org/apache/hc/core5/http/impl/nio/ServerHttp1StreamDuplexer.java#L439-L447 This caused the next request on that connection to never be considered "active", so it would never send data to the client. I reproduced this case by simulating a race condition between the application thread calling `ResponseTrigger.submitResponse()` to submit an response asynchronously and the IO worker thread handling the "Output Ready" event. If you enforce the timing between 1 and 3, the repro is guaranteed. prereq: requires HTTP response to have no body 1. App thread submits response and pauses before executing ServerHttp1StreamHandler#196 2. IO thread begins executing outputReady stack. 3. IO thread does not execute IF at ServerHttp1StreamDuplexer#439 because outgoing.isResponseFinal() returns false 4. Application thread resumes and sets the completed flag. This is its last action. 5. IO thread resumes This PR contains that change I've deployed in our environment and appears to solve the issue. --- Aside: I'm 95% sure the presence of this code, and thus my adaptation of it, is also a bug: https://github.com/apache/httpcomponents-core/blob/5aad6bb138372c2ed5ffd8d9317cff49dd17c2dd/httpcore5/src/main/java/org/apache/hc/core5/http/impl/nio/ServerHttp1StreamHandler.java#L199 As I have to use a custom BasicResponseProducer with a synchronized `produce()` method to avoid fake Connection Closed exceptions when multiple threads execute: https://github.com/apache/httpcomponents-core/blob/5aad6bb138372c2ed5ffd8d9317cff49dd17c2dd/httpcore5/src/main/java/org/apache/hc/core5/http/nio/entity/BasicAsyncEntityProducer.java#L130-L132 The two threads in question are again the application thread submitting the response async and the IO worker thread. Multiple threads can enter the if because the `bytebuf.hasRemaining()` is not atomic WRT reading from the buffer inside `write()`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@hc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@hc.apache.org For additional commands, e-mail: dev-h...@hc.apache.org
