Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 414d6e28 authored by Nicolo' Mazzucato's avatar Nicolo' Mazzucato
Browse files

Fix race from DisplayRepository.getDisplay 

Before this cl getDisplay(id) was guaranteed to not cause any binder call, as the mapping between displayId and Display in DisplayRepository was done in the background.

However, this was causing getDisplay(id) to possibly return null after a display was added, in the case the binder call to get the display instance was in flight while getDisplay was called.

This cl regretfully makes getDisplay return the cached Display instance if available, or makes it trigger a binder call to get the Display if we know its id was added already. While this might cause regressions, it's necessary to ensure correctness. Anyway, the binder call shouldn't happen more than once per display id, so I don't expect the impact of this to be that high.

A new method with the old non-blocking behaviour has been added: getCachedDisplay.

Bug: 362719719
Bug: 410580872
Test: DisplayRepositoryTest
Flag: NONE - small performance bugfix
Change-Id: Iaf41e17222315359c01d8cf3590dc8e3d177d01d
parent 0c4aeff0

packages/SystemUI/multivalentTests/src/com/android/systemui/display/data/repository/DisplayRepositoryTest.kt

+14 −0
Original line number Diff line number Diff line
@@ -638,6 +638,20 @@ class DisplayRepositoryTest : SysuiTestCase() { 
        verify(commandQueue, times(1)).removeCallback(any())

    }



    @Test

    fun getDisplay_slowMappingToDisplay_returnsRegardless() =

        testScope.runTest {

            val displayIds by collectLastValue(displayRepository.displayIds)

            val displays by latestDisplayFlowValue()



            sendOnDisplayAdded(1, TYPE_EXTERNAL)



            assertThat(displayIds).contains(1)

            assertThat(displays!!.ids()).contains(1)

            assertThat(displayRepository.getCachedDisplay(1)).isNotNull()

            assertThat(displayRepository.getDisplay(1)).isNotNull()

        }



    private fun Iterable<Display>.ids(): List<Int> = map { it.displayId }



    private fun Iterable<Set<Display>>.toIdSets(): List<Set<Int>> = map { it.ids().toSet() }

packages/SystemUI/src/com/android/systemui/display/data/repository/PerDisplayStore.kt

+1 −1
Original line number Diff line number Diff line
@@ -68,7 +68,7 @@ abstract class PerDisplayStoreImpl<T>( 
     *   displays.

     */

    override fun forDisplay(displayId: Int): T? {

        if (displayRepository.getDisplay(displayId)  == null) {

        if (!displayRepository.containsDisplay(displayId)) {

            Log.e(TAG, "<${instanceClass.simpleName}>: Display with id $displayId doesn't exist.")

            return null

        }

packages/SystemUI/tests/utils/src/com/android/systemui/display/data/repository/FakeDisplayRepository.kt

+4 −0
Original line number Diff line number Diff line
@@ -116,6 +116,10 @@ class FakeDisplayRepository @Inject constructor() : DisplayRepository { 
    override val defaultDisplayOff: Flow<Boolean>

        get() = _defaultDisplayOff.asStateFlow()



    override fun getDisplay(displayId: Int): Display? {

        return displays.value.find { it.displayId == displayId }

    }



    override val displayAdditionEvent: Flow<Display?>

        get() = displayAdditionEventFlow