[HTTPCLIENT-2033] Connection manager shutdown flushes output buffer without synchronization

• Type: Bug
• Status: Closed
• Priority: Major
• Resolution: Fixed
• Affects Version/s: 4.5.10
• Fix Version/s: 4.5.11
• Component/s: HttpClient (classic)
• Environment: Linux
• Assignee: Unassigned
• Reporter: Aapo Laitinen
• Created: 04/Dec/19 08:52
• Updated: 09/Dec/19 04:06
• Resolved: 05/Dec/19 08:48

Description

We've observed cases of garbled data sent over the wire that seem to correlate with calling HttpClientConnectionManager#shutdown() while requests are in progress. For example, a PUT request with entity ABCD (each letter corresponding to 4 kilobytes of data) may be received as ABCB (the last block replaced with the second block).

Omitting some intermediate layers, HttpClientConnectionManager#shutdown() proceeds to call AbstractConnPool#shutdown(), BHttpConnectionBase#close(), SessionOutputBufferImpl#flush() and finally SessionOutputBufferImpl#flushBuffer(). As far as I can tell, there is nothing to stop the thread that is performing the shutdown from invoking flushBuffer() while another thread is writing data to the buffer and/or flushing it.

How to reproduce

I haven't been able to reliably reproduce the garbled data, but I confirmed the lack of synchronization as follows:

1. Modified flushBuffer() so that it will try to a acquire and release a shared lock before proceeding and will report any cases of two threads attempting to flush the same buffer at the same time.
2. Acquired the lock while another thread was making requests, making the other thread block inside flushBuffer().
3. Called HttpClientConnectionManager#shutdown()
4. Observed that the thread doing the shutdown entered flushBuffer() while the thread that was making requests hadn't exited it yet.

What I was expecting to happen

I'm not sure what the expected behavior of calling HttpClientConnectionManager#shutdown() is, but either of flushing the buffer with proper synchronization or abruptly closing the connection (BHttpConnectionBase#shutdown() instead of close()) strike me as acceptable. Basically, I'd expect that the received data might be truncated (e.g. AB or ABC instead of ABCD) but never garbled (e.g. ABCB or ABCC instead of ABCD).