Add missing synchronization in some methods

app/src/main/java/com/emanuelef/remote_capture/ConnectionsRegister.java

@@ -39,6 +39,20 @@ import java.util.Collections;
 import java.util.List;
 import java.util.Set;
 
+/* A container for the connections. This is used to store active/closed connections until the capture
+ * is stopped. Active connections are also kept in the native side.
+ *
+ * The ConnectionsRegister can store up to _size items, after which rollover occurs and older
+ * connections are replaced with the new ones. Via the addListener method it's possible to listen
+ * for connections changes (connections added/removed/updated). The usual listener for such events
+ * is the ConnectionsFragment, which then forwards them to the ConnectionsAdapter.
+ *
+ * Connections are added/updated by the CaptureService in a separate thread. The getter methods are
+ * instead called on the UI thread, usually by the ConnectionsAdapter. Methods are synchronized to
+ * provide threads safety on this class. Concurrent access to the ConnectionDescriptors fields can
+ * occur during connectionsUpdates but it's not protected, check out the ConnectionDescriptor class
+ * for more details.
+ */
 public class ConnectionsRegister {
     private static final String TAG = "ConnectionsRegister";
 
@@ -46,7 +60,7 @@ public class ConnectionsRegister {
     private int mTail;
     private final int mSize;
     private int mNumItems;
-    private int mUntrackedItems;
+    private int mUntrackedItems; // number of old connections which were discarded due to the rollover
     private int mNumMalicious;
     private final SparseArray<AppStats> mAppsStats;
     private final SparseIntArray mConnsByIface;
@@ -65,11 +79,13 @@ public class ConnectionsRegister {
         mConnsByIface = new SparseIntArray();
     }
 
-    private int firstPos() {
+    // returns the position in mItemsRing of the oldest connection
+    private synchronized int firstPos() {
         return (mNumItems < mSize) ? 0 : mTail;
     }
 
-    private int lastPos() {
+    // returns the position in mItemsRing of the newest connection
+    private synchronized int lastPos() {
         return (mTail - 1 + mSize) % mSize;
     }
 
@@ -87,6 +103,7 @@ public class ConnectionsRegister {
         }
     }
 
+    // called by the CaptureService in a separate thread when new connections should be added to the register
     public synchronized void newConnections(ConnectionDescriptor[] conns) {
         if(conns.length > mSize) {
             // take the most recent
@@ -178,6 +195,7 @@ public class ConnectionsRegister {
         }
     }
 
+    // called by the CaptureService in a separate thread when connections should be updated
     public synchronized void connectionsUpdates(ConnectionUpdate[] updates) {
         int first_pos = firstPos();
         int first_id = mItemsRing[first_pos].incr_id;
@@ -256,7 +274,8 @@ public class ConnectionsRegister {
         return mUntrackedItems;
     }
 
-    public @Nullable ConnectionDescriptor getConn(int i) {
+    // get the i-th oldest connection
+    public synchronized @Nullable ConnectionDescriptor getConn(int i) {
         if((i < 0) || (i >= mNumItems))
             return null;
 
@@ -271,9 +290,8 @@ public class ConnectionsRegister {
             int pos = (first + i) % mSize;
             ConnectionDescriptor item = mItemsRing[pos];
 
-            if((item != null) && (item.incr_id == incr_id)) {
+            if((item != null) && (item.incr_id == incr_id))
                 return pos;
-            }
         }
 
         return -1;
@@ -305,12 +323,12 @@ public class ConnectionsRegister {
         return mNumMalicious;
     }
 
-    public boolean hasSeenMultipleInterfaces() {
+    public synchronized boolean hasSeenMultipleInterfaces() {
         return(mConnsByIface.size() > 1);
     }
 
     // Returns a sorted list of seen network interfaces
-    public List<String> getSeenInterfaces() {
+    public synchronized List<String> getSeenInterfaces() {
         List<String> rv = new ArrayList<>();
 
         for(int i=0; i<mConnsByIface.size(); i++) {

app/src/main/java/com/emanuelef/remote_capture/adapters/ConnectionsAdapter.java

@@ -231,6 +231,7 @@ public class ConnectionsAdapter extends RecyclerView.Adapter<ConnectionsAdapter.
     }
 
     private void removeFilteredItemAt(int pos) {
+        // get the previous item which was now removed
         ConnectionDescriptor item = getItem(pos);
         if(item == null)
             return;
@@ -321,7 +322,6 @@ public class ConnectionsAdapter extends RecyclerView.Adapter<ConnectionsAdapter.
 
         for(int reg_pos : positions) {
             ConnectionDescriptor conn = reg.getConn(reg_pos);
-
             if(conn != null) {
                 // reg_pos is the position in the ConnectionsRegister, whereas pos is the position
                 // in mFilteredConn
@@ -357,7 +357,6 @@ public class ConnectionsAdapter extends RecyclerView.Adapter<ConnectionsAdapter.
 
     public void refreshFilteredConnections() {
         final ConnectionsRegister reg = CaptureService.getConnsRegister();
-
         if(reg == null)
             return;
 
@@ -369,12 +368,15 @@ public class ConnectionsAdapter extends RecyclerView.Adapter<ConnectionsAdapter.
             int pos = 0;
             mFilteredConn = new ArrayList<>();
 
-            for(int i=0; i<mUnfilteredItemsCount; i++) {
-                ConnectionDescriptor conn = reg.getConn(i);
+            // Synchronize to improve performance of getConn
+            synchronized(reg) {
+                for(int i = 0; i < mUnfilteredItemsCount; i++) {
+                    ConnectionDescriptor conn = reg.getConn(i);
 
-                if(matches(conn)) {
-                    mFilteredConn.add(conn);
-                    mIdToFilteredPos.put(conn.incr_id, pos++);
+                    if(matches(conn)) {
+                        mFilteredConn.add(conn);
+                        mIdToFilteredPos.put(conn.incr_id, pos++);
+                    }
                 }
             }
 
@@ -420,7 +422,7 @@ public class ConnectionsAdapter extends RecyclerView.Adapter<ConnectionsAdapter.
         return (mSearch != null) || mFilter.isSet();
     }
 
-    public synchronized String dumpConnectionsCsv() {
+    public String dumpConnectionsCsv() {
         StringBuilder builder = new StringBuilder();
         AppsResolver resolver = new AppsResolver(mContext);
 

app/src/main/java/com/emanuelef/remote_capture/fragments/ConnectionsFragment.java

@@ -510,9 +510,7 @@ public class ConnectionsFragment extends Fragment implements ConnectionsListener
         mMenuItemSearch.expandActionView();
 
         // Delay otherwise the query won't be set when the activity is just started.
-        mSearchView.post(() -> {
-            mSearchView.setQuery(query, true);
-        });
+        mSearchView.post(() -> mSearchView.setQuery(query, true));
     }
 
     private void recheckScroll() {
@@ -614,7 +612,7 @@ public class ConnectionsFragment extends Fragment implements ConnectionsListener
     }
 
     @Override
-    public void connectionsRemoved(int start,ConnectionDescriptor []conns) {
+    public void connectionsRemoved(int start, ConnectionDescriptor []conns) {
         mHandler.post(() -> {
             Log.d(TAG, "Remove " + conns.length + " connections at " + start);
             mAdapter.connectionsRemoved(start, conns);

app/src/main/java/com/emanuelef/remote_capture/model/ConnectionDescriptor.java

@@ -30,10 +30,16 @@ import java.io.Serializable;
 import java.net.InetAddress;
 import java.net.UnknownHostException;
 
-/* Equivalent of zdtun_conn_t from zdtun and pd_conn_t from pcapdroid.c */
+/* Holds the information about a single connection.
+ * Equivalent of zdtun_conn_t from zdtun and pd_conn_t from pcapdroid.c .
+ *
+ * Connections are normally stored into the ConnectionsRegister. Concurrent access to the connection
+ * fields can happen when a connection is updated and, at the same time, it is retrieved by the UI
+ * thread. However this does not create concurrency problems as the update only increments counters
+ * or sets a previously null field to a non-null value.
+ */
 public class ConnectionDescriptor implements Serializable {
     // sync with zdtun_conn_status_t
-
     public static final int CONN_STATUS_NEW = 0,
         CONN_STATUS_CONNECTING = 1,
         CONN_STATUS_CONNECTED = 2,
@@ -44,6 +50,7 @@ public class ConnectionDescriptor implements Serializable {
         CONN_STATUS_RESET = 7,
         CONN_STATUS_UNREACHABLE = 8;
 
+    // This is an high level status which abstracts the zdtun_conn_status_t
     public enum Status {
         STATUS_INVALID,
         STATUS_OPEN,

app/src/test/java/com/emanuelef/remote_capture/adapters/ConnectionsAdapterTest.java

@@ -170,6 +170,8 @@ public class ConnectionsAdapterTest {
                 newConnection(true),
                 newConnection(false),
         });
+        assertEvent(ChangeType.ITEMS_INSERTED, 0, 6);
+
         // add 4 connections, 2 of which replace the first 2
         reg.newConnections(new ConnectionDescriptor[] {
                 newConnection(false),