Commit 7611dfd7 authored by Grisha Kruglov's avatar Grisha Kruglov
Browse files

Pre 2: remove unnecessary synchronization from FxaDeviceConstellation 

I'm not sure it was ever necessary, to be honest. I think the way the 'refresh'
method is currently structured, simply marking internal 'constellationState'
as Volatile is enough - we're not concerned about concurrent access to it,
only about memory visibility across threads.

It's true that currently, work in this class happens on a pool thread,
so it's possible to have two racing 'refresh' methods running. Having one of them
loose the race and drop results on the floor should be just fine in this case.
parent 778a2052

components/service/firefox-accounts/src/main/java/mozilla/components/service/fxa/FxaDeviceConstellation.kt

+24 −29
Original line number Diff line number Diff line
@@ -5,7 +5,6 @@ 
package mozilla.components.service.fxa



import android.content.Context

import androidx.annotation.GuardedBy

import androidx.lifecycle.LifecycleOwner

import kotlinx.coroutines.CoroutineScope

import kotlinx.coroutines.Deferred
@@ -39,12 +38,10 @@ class FxaDeviceConstellation( 


    private val deviceObserverRegistry = ObserverRegistry<DeviceConstellationObserver>()



    @GuardedBy("this")

    @Volatile

    private var constellationState: ConstellationState? = null



    override fun state(): ConstellationState? = synchronized(this) {

        return constellationState

    }

    override fun state(): ConstellationState? = constellationState



    override fun initDeviceAsync(

        name: String,
@@ -137,38 +134,36 @@ class FxaDeviceConstellation( 
        notifyObservers { onEvents(events) }

    }



    override fun refreshDevicesAsync(): Deferred<Boolean> = synchronized(this) {

        return scope.async {

    override fun refreshDevicesAsync(): Deferred<Boolean> = scope.async {

        logger.info("Refreshing device list...")



        // Attempt to fetch devices, or bail out on failure.

        val allDevices = fetchAllDevicesAsync().await() ?: return@async false



        // Find the current device.

            val currentDevice = allDevices.find { it.isCurrentDevice }

        val currentDevice = allDevices.find { it.isCurrentDevice }?.also {

            // Check if our current device's push subscription needs to be renewed.

            if (it.subscriptionExpired) {

                logger.info("Current device needs push endpoint registration")

            }

        }



        // Filter out the current devices.

        val otherDevices = allDevices.filter { !it.isCurrentDevice }



        val newState = ConstellationState(currentDevice, otherDevices)

        constellationState = newState



        logger.info("Refreshed device list; saw ${allDevices.size} device(s).")



        CoroutineScope(Dispatchers.Main).launch {

            // NB: at this point, 'constellationState' might have changed.

            // Notify with an immutable, local 'newState' instead.

            deviceObserverRegistry.notifyObservers { onDevicesUpdate(newState) }

        }



            // Check if our current device's push subscription needs to be renewed.

            constellationState?.currentDevice?.let {

                if (it.subscriptionExpired) {

                    logger.info("Current device needs push endpoint registration")

                }

            }



            logger.info("Refreshed device list; saw ${allDevices.size} device(s).")

        true

    }

    }



    /**

     * Get all devices in the constellation.