Make MediaHTTPConnection thread safe.
MediaHTTPConnection's public methods are called from multiple Binder threads. Since both HttpURLConnection and access to the various connection related fields is not thread safe, this CL guards most methods by a single lock. This means that the methods can now block when called, although this should be rare: - there are two processes that call these methods. One process only calls getSize(), and the other process calls methods from a single thread (ie. at not overlapping clock times). - should lock contention unexpectedly increase in future, then that would be bad (because Binder thread pool threads would be blocked/unavailable), but it would not be easy to detect. It would be easy to detect if we could stop getSize() being called at overlapping clock times, since we could then use ReentrantLock.tryLock() to assert that the lock is never contended outside of disconnect(). Because it's a requirement for disconnect() to quickly stop another thread that is blocked in readAt(), disconnect() is the only method that doesn't acquire the lock immediately; the mConnection field is marked volatile so that disconnect() has a high chance of reading that field and calling disconnect() on it without waiting for another thread (there's a small risk that another thread might acquire the lock and start a new connection while disconnect() is waiting for the lock; in that case, after acquiring the lock, disconnect() will also disconnect that new connection; this is subject to potential change in future. Initially, a ReentrantLock object was considered but for now this CL instead uses the synchronized lock on "this" because: - it minimizes churn on the lines of code in this file because synchronized (this) { } can be expressed by introduction of the word "synchronized" on the method header, whereas mLock.lock(); try { ... } finally { mLock.unlock(); } would indent all the lines in-between and thus pollute git annotate. - some methods were already synchronized. - ReentrantLock.tryLock() is not used for now; most of the time, lock acquisition should be uncontended but the two cases of lock contention mentioned above exist, which makes it difficult to distinguish surprising from unsurprising lock contention. While this is the case, it seems better to keep the code simple and to just unconditionally block. Bug: 114337214 Fixes: 114337214 Fixes: 119900000 Fixes: 129444137 Fixes: 128758794 Fixes: 63002045 Test: Checked manually that bug 114337214 no longer reproduces on Android API level 27 (Oreo MR1) after cherrypicking this CL. Test: Ran the following on internal master with this CL: make cts && cts-tradefed run cts -m CtsMediaTestCases \ -t android.media.cts.NativeDecoderTest#testAMediaDataSourceClose \ --abi arm64-v8a Test: Ran the following both on AOSP (158 tests) and internal master (178): atest CtsMediaTestCases:android.media.cts.{MediaPlayer{,2},Routing}Test All these tests pass except that on AOSP only, the following test fails both before and after my CL (appears unrelated): android.media.cts.RoutingTest#test_MediaPlayer_RoutingChangedCallback (cherry picked from commit 8d9fccee) Change-Id: I4e32a58891c3ce60ddfa72d36060486d37906f8d Merged-In: I4e32a58891c3ce60ddfa72d36060486d37906f8d
Loading
Please register or sign in to comment