am: d4b3f65d35 -s ours
am skip reason: change_id I9cb569bfd1ef5fba7958bb2e5f42f05e300e3358 with SHA1 fa9460a6f0 is in history
Change-Id: Ia51fc2606a1ae6e97247f07444e4a5cd7a4629f2
am: 253a139102 -s ours
am skip reason: change_id If786d51b35380f606bc388e29a441bb24a2792e0 with SHA1 0c92c23951 is in history
Change-Id: I44cdc1f9a77cab58c341a1cd03c7551e99946a14
am: 24fc368617 -s ours
am skip reason: change_id I9e8449e5a75db616e646f55c930ff82982fc9083 with SHA1 d78542bb52 is in history
Change-Id: Ia496e4e78cec60895d792976a47ff02a799bb03f
am: ee6ed6914f -s ours
am skip reason: change_id Ifcfe4df81caf8ede2e4e66a76552cb3200378fa8 with SHA1 061cec7755 is in history
Change-Id: I419ae9923a510c9691c8c88911ac45fe76846166
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 8d9fccee62e2c73abe952f2a1de575c28bcd9410)
Change-Id: I4e32a58891c3ce60ddfa72d36060486d37906f8d
Merged-In: I4e32a58891c3ce60ddfa72d36060486d37906f8d
This reverts commit 621e7968adf0253d5e22406f02ccc8bcc0eda5ec.
Many of the fields that were moved are annotated @UnsupportedAppUsage,
so the CL would have had undesirable app compat impact. Further,
because investigation has revealed that lock contention *is* possible,
we need to always acquire the lock anyway so there is no longer a
benefit in keeping all of the mutable state in a single field that
can be atomically set to null.
Bug: 114337214
Test: Treehugger
(cherry picked from commit dc9f4b4d5d28fc68b1b5e4e8500bf67d4b11621d)
Change-Id: I202c5653cb086d99228491e161a159bad640105a
Merged-In: I202c5653cb086d99228491e161a159bad640105a
