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

Commit ccd79ce9 authored by Rambo Wang's avatar Rambo Wang
Browse files

CellularDataService: mCallbackMap is not thread safe 

The HashMap object mCallbackMap in CellularDataService is a shared
resource between DataService's handler thread and CellularDataService's
handler thread. The update operations (put and remove) has
synchronization issue.

This CL fixes the issue by letting CellularDataService shares the
looper/handler thread of DataService (but keeping its own handler). And
thus all the operations of mCallbackMap are handled serializally in
DataService's handler thread.

One alternative solution is to  move all operations of the mCallbackMap into
CellularDataService's handler thread to make it thread-safe. But it's
difficult for close() method which need to clear mCallbackMap and
shutdown the handler thread.

The other alternative solution is to replace HashMap with ConcurrentHashMap
for mCallbackMap. This do make mCallbackMap thread-safe but do not
change the fact that it is still a shared resource between two threads.

Bug: 151103522
Test: atest FrameworksTelephonyTests
Change-Id: Id0f830b27b0b64e83a937b76575bd9497e56cd03
parent 3e5e1a76

src/java/com/android/internal/telephony/CellularNetworkService.java

+4 −13
Original line number Diff line number Diff line
@@ -20,7 +20,6 @@ import android.hardware.radio.V1_0.RegState; 
import android.hardware.radio.V1_4.DataRegStateResult.VopsInfo.hidl_discriminator;

import android.os.AsyncResult;

import android.os.Handler;

import android.os.HandlerThread;

import android.os.Looper;

import android.os.Message;

import android.telephony.AccessNetworkConstants;
@@ -37,8 +36,9 @@ import android.telephony.TelephonyManager; 
import com.android.telephony.Rlog;



import java.util.ArrayList;

import java.util.HashMap;

import java.util.List;

import java.util.concurrent.ConcurrentHashMap;

import java.util.Map;



/**

 * Implementation of network services for Cellular. It's a service that handles network requests
@@ -56,12 +56,7 @@ public class CellularNetworkService extends NetworkService { 


    private class CellularNetworkServiceProvider extends NetworkServiceProvider {



        private final ConcurrentHashMap<Message, NetworkServiceCallback> mCallbackMap =

                new ConcurrentHashMap<>();



        private final Looper mLooper;



        private final HandlerThread mHandlerThread;

        private final Map<Message, NetworkServiceCallback> mCallbackMap = new HashMap<>();



        private final Handler mHandler;
@@ -72,10 +67,7 @@ public class CellularNetworkService extends NetworkService { 


            mPhone = PhoneFactory.getPhone(getSlotIndex());



            mHandlerThread = new HandlerThread(CellularNetworkService.class.getSimpleName());

            mHandlerThread.start();

            mLooper = mHandlerThread.getLooper();

            mHandler = new Handler(mLooper) {

            mHandler = new Handler(Looper.myLooper()) {

                @Override

                public void handleMessage(Message message) {

                    NetworkServiceCallback callback = mCallbackMap.remove(message);
@@ -377,7 +369,6 @@ public class CellularNetworkService extends NetworkService { 
        @Override

        public void close() {

            mCallbackMap.clear();

            mHandlerThread.quit();

            mPhone.mCi.unregisterForNetworkStateChanged(mHandler);

        }

    }

src/java/com/android/internal/telephony/dataconnection/CellularDataService.java

+1 −10
Original line number Diff line number Diff line
@@ -19,7 +19,6 @@ package com.android.internal.telephony.dataconnection; 
import android.net.LinkProperties;

import android.os.AsyncResult;

import android.os.Handler;

import android.os.HandlerThread;

import android.os.Looper;

import android.os.Message;

import android.telephony.SubscriptionManager;
@@ -56,12 +55,8 @@ public class CellularDataService extends DataService { 


        private final Map<Message, DataServiceCallback> mCallbackMap = new HashMap<>();



        private final Looper mLooper;



        private final Handler mHandler;



        private final HandlerThread mHandlerThread;



        private final Phone mPhone;



        private CellularDataServiceProvider(int slotId) {
@@ -69,10 +64,7 @@ public class CellularDataService extends DataService { 


            mPhone = PhoneFactory.getPhone(getSlotIndex());



            mHandlerThread = new HandlerThread(CellularDataService.class.getSimpleName());

            mHandlerThread.start();

            mLooper = mHandlerThread.getLooper();

            mHandler = new Handler(mLooper) {

            mHandler = new Handler(Looper.myLooper()) {

                @Override

                public void handleMessage(Message message) {

                    DataServiceCallback callback = mCallbackMap.remove(message);
@@ -206,7 +198,6 @@ public class CellularDataService extends DataService { 
        @Override

        public void close() {

            mPhone.mCi.unregisterForDataCallListChanged(mHandler);

            mHandlerThread.quit();

        }

    }