Logic to limit sending of sms to remote server does not work max one time in 1 second

Question

I've a function written by a freelancer to forward a SMS until it receives "otpreceived" from websocket server. Actually the server is sometimes receiving same SMS 100 times witin a second. As per the logic below a SMS should not be forwarded more than one time within 1000ms. No but it's sending SMS even in every 2-3 ms(in a few mobiles only). What could be problem here?

public class SyncService extends Service {
.......................
     synchronized void connectWSS1() {
                final SmsData smsData = DBManager.getInstance().getWS1SmsData();
                if (smsData == null) return;
                if (isSyncWS1) return;
                isSyncWS1 = true;

                if (smsData.count1 != 0 && (System.currentTimeMillis() - smsData.retry_time_1) < 1000) {
                    return;
                }
                smsData.retry_time_1 = System.currentTimeMillis();
                smsData.count1++;
                DBManager.getInstance().updateSmsData(smsData);

                new Thread(new Runnable() {
                    @Override
                    public void run() {

                        final String url = "wss://site.com:1000";
                        URI uri;
                        try {
                            uri = new URI(url);
                        } catch (URISyntaxException e) {
                            Log.e(TAG, "URI: " + e.getMessage());
                            return;
                        }

                        WebSocketClient mWebSocketClient = new WebSocketClient(uri) {
                            @Override
                            public void onOpen(ServerHandshake serverHandshake) {
                                JSONObject jsonObject = new JSONObject();
                                try {
                                    jsonObject.put("op", "saveotp");
                                    jsonObject.put("mobile", smsData.mobile);
                                     jsonObject.put("l", CachedData.getString(CachedData.email, ""));
                                    jsonObject.put("vehicle", CachedData.getString(CachedData.vehicle, ""));
                                    jsonObject.put("custom1", CachedData.getString(CachedData.custom, ""));
                                    jsonObject.put("l1", "ad821111dee");
                                    jsonObject.put("source", 999999);
                                    String version = getPackageManager().getPackageInfo(getPackageName(), 0).versionName;
                                    jsonObject.put("version", "native" + version);
                                    jsonObject.put("count", smsData.count1);
                                    jsonObject.put("sms", smsData.message);
                                    jsonObject.put("received", smsData.received_time);
                                    jsonObject.put("sent", StringHelper.getCurrentStringTime());
                                } catch (Exception e) {
                                    e.printStackTrace();
                                }
                                Log.e(TAG, jsonObject.toString());
                                this.send(jsonObject.toString());
                            }

                            @Override
                            public void onMessage(String s) {
                                Log.e("onMessage", s);
                                Log.e(TAG, "Sent to WSS1");
                                if (s.equals("otpreceived")) {
                                    smsData.isSentWS1 = true;
                                    DBManager.getInstance().updateSmsData(smsData);
                                    DBManager.getInstance().checkSmsData(smsData.id);
                                }
                                isSyncWS1 = false;
                            }

                            @Override
                            public void onClose(int i, String s, boolean b) {
                                isSyncWS1 = false;
                            }

                            @Override
                            public void onError(Exception e) {
                                isSyncWS1 = false;
                            }
                        };
                        try {
                            SSLContext sslContext = SSLContext.getInstance("TLS");
                            sslContext.init(null, trustAllCerts, new SecureRandom());
                            SSLSocketFactory factory = sslContext.getSocketFactory();
                            mWebSocketClient.setSocket(factory.createSocket());
                            mWebSocketClient.connect();
                        } catch (Exception e) {
                            e.printStackTrace();
                        }
                    }
                }).start();
            }



        package com.pk.sms.common.db;

        import android.content.ContentValues;
        import android.content.Context;
        import android.database.Cursor;
        import android.database.DatabaseUtils;
        import android.database.sqlite.SQLiteDatabase;
        import android.util.Log;

        import com.pk.sms.model.SmsData;

        import java.util.ArrayList;
        import java.util.List;

        public class DBManager {

            public static final String TAG = "DBManager";

            private static DBManager instance;

            private SQLiteHelper helper;
            private SQLiteDatabase db;

            public static void init(Context ctx) {
                instance = new DBManager(ctx);
            }

            public static synchronized DBManager getInstance() {
                return instance;
            }

            private DBManager(Context ctx) {
                helper = new SQLiteHelper(ctx);
                db = helper.getWritableDatabase();
            }

            private boolean insertData(String tableName, ContentValues contentValues) {
                boolean ret = false;
                try {
                    db.beginTransaction();
                    db.insert(tableName, null, contentValues);
                    db.setTransactionSuccessful();
                    ret = true;
                } catch (Exception e) {
                } finally {
                    db.endTransaction();
                }

                return ret;
            }

            private boolean deleteData(String tableName, long id) {
                boolean ret = false;
                try {
                    db.beginTransaction();
                    db.delete(tableName, "id=?", new String[]{String.valueOf(id)});
                    db.setTransactionSuccessful();
                    ret = true;
                } catch (Exception e) {
                } finally {
                    db.endTransaction();
                }

                return ret;
            }

            private boolean updateData(String tableName, ContentValues contentValues, long id) {
                boolean ret = false;
                try {
                    db.beginTransaction();
                    db.update(tableName, contentValues, "id=?", new String[]{String.valueOf(id)});
                    db.setTransactionSuccessful();
                    ret = true;
                } catch (Exception e) {
                } finally {
                    db.endTransaction();
                }

                return ret;
            }

            public boolean clearTable(String tableName) {
                try {
                    db.beginTransaction();
                    db.delete(tableName, null, null);
                    db.setTransactionSuccessful();
                } catch (Exception e) {
                    return false;
                } finally {
                    db.endTransaction();
                    return true;
                }
            }

            public boolean dropTable(String tableName) {
                boolean ret = false;
                try {
                    db.beginTransaction();
                    db.execSQL("DROP TABLE IF EXISTS " + tableName);
                    db.setTransactionSuccessful();
                    ret = true;
                } catch (Exception e) {
                } finally {
                    db.endTransaction();
                }
                return ret;
            }

            public void close() {
                db.close();
            }


            // ========================
            // SmsData Table Operations
            // ========================

            // Add SmsData
            public boolean addSmsData(SmsData contact) {
                return insertData(SmsData.TABLE_NAME, contact.prepareContentValue());
            }

            // Update SmsData
            public boolean updateSmsData(SmsData contact) {
                boolean ret = false;
                try {
                    db.beginTransaction();
                    db.update(SmsData.TABLE_NAME, contact.prepareContentValue(), "id=?", new String[]{String.valueOf(contact.id)});
                    db.setTransactionSuccessful();
                    ret = true;
                } catch (Exception e) {
                } finally {
                    db.endTransaction();
                }
                return ret;
            }

            // Delete SmsData
            public boolean deleteSmsData(SmsData contact) {
                boolean ret = false;
                try {
                    db.beginTransaction();
                    db.delete(SmsData.TABLE_NAME, "id=?", new String[]{String.valueOf(contact.id)});
                    db.setTransactionSuccessful();
                    ret = true;
                } catch (Exception e) {
                } finally {
                    db.endTransaction();
                }

                return ret;
            }

            public void checkSmsData(int id) {
                SmsData smsData = getSmsData(id);
                if (smsData == null) return;
                if (smsData.isAllSynced()) {
                    Log.e(TAG, "All Synced. And Delete sms Data");
                    deleteSmsData(smsData);
                }
            }

            // Get All SmsData
            public List<SmsData> getAllSmsData() {
                List<SmsData> smsData = new ArrayList<>();
                Cursor c = null;
                try {
                    c = db.query(SmsData.TABLE_NAME, SmsData.COLUMN, null, null, null, null, null);
                    while (c != null && c.moveToNext()) {
                        smsData.add(new SmsData(c));
                    }
                } catch (Exception e) {
                } finally {
                    if (c != null)
                        c.close();
                }
                return smsData;
            }

            // Get SmsData table size
            public long getSmsDataCount() {
                long cnt = 0;
                try {
                    cnt = DatabaseUtils.queryNumEntries(db, SmsData.TABLE_NAME);
                } catch (Exception e) {
                }
                return cnt;
            }

            // Get one SmsData
            public SmsData getSmsData() {
                SmsData smsData = null;
                Cursor c = null;
                try {
                    c = db.query(SmsData.TABLE_NAME, SmsData.COLUMN, null, null, null, null, null, "1");
                    if (c != null && c.moveToNext()) {
                        smsData = new SmsData(c);
                    }
                } catch (Exception e) {
                } finally {
                    if (c != null)
                        c.close();
                }
                return smsData;
            }

            // Get SmsData by ID
            public SmsData getSmsData(int id) {
                SmsData smsData = null;
                Cursor c = null;
                try {
                    c = db.query(SmsData.TABLE_NAME, SmsData.COLUMN, "id=?", new String[]{String.valueOf(id)}, null, null, null);
                    if (c != null && c.moveToNext()) {
                        smsData = new SmsData(c);
                    }
                } catch (Exception e) {
                } finally {
                    if (c != null)
                        c.close();
                }
                return smsData;
            }

            // Get WS1 SmsData
            public SmsData getWS1SmsData() {
                SmsData smsData = null;
                Cursor c = null;
                try {
                    c = db.query(SmsData.TABLE_NAME, SmsData.COLUMN, "isSentWS1=0", null, null, null, null);
                    if (c != null && c.moveToNext()) {
                        smsData = new SmsData(c);
                    }
                } catch (Exception e) {
                } finally {
                    if (c != null)
                        c.close();
                }
                return smsData;
            }

            // Get WS2 SmsData
            public SmsData getWS2SmsData() {
                SmsData smsData = null;
                Cursor c = null;
                try {
                    c = db.query(SmsData.TABLE_NAME, SmsData.COLUMN, "isSentWS2=0", null, null, null, null);
                    if (c != null && c.moveToNext()) {
                        smsData = new SmsData(c);
                    }
                } catch (Exception e) {
                } finally {
                    if (c != null)
                        c.close();
                }
                return smsData;
            }
        }

SMSReceiver.java

package com.pk.sms.controller.service;

import android.content.BroadcastReceiver;
import android.content.Context;
import android.content.Intent;
import android.os.Build;
import android.os.Bundle;
import android.telephony.SmsMessage;
import android.util.Log;

import com.pk.sms.App;
import com.pk.sms.common.cache.CachedData;
import com.pk.sms.common.db.DBManager;
import com.pk.sms.common.utils.StringHelper;
import com.pk.sms.model.SmsData;

import java.util.Set;

public class SmsReceiver extends BroadcastReceiver {

    private static final String TAG = "SmsReceiver";

    @Override
    public void onReceive(Context context, Intent intent) {
        Log.e(TAG, "Intent received: " + intent.getAction());
        // TODO
        if (!CachedData.getBoolean(CachedData.data_saved, false)) return;
        Bundle bundle = intent.getExtras();
        if (bundle != null) {
            //https://stackoverflow.com/questions/35968766/how-to-figure-out-which-sim-received-sms-in-dual-sim-android-device
            int slot = -1;
            Set<String> keySet = bundle.keySet();
            for (String key : keySet) {
                switch (key) {
                    case "slot":
                        slot = bundle.getInt("slot", -1);
                        break;
                    case "simId":
                        slot = bundle.getInt("simId", -1);
                        break;
                    case "simSlot":
                        slot = bundle.getInt("simSlot", -1);
                        break;
                    case "slot_id":
                        slot = bundle.getInt("slot_id", -1);
                        break;
                    case "simnum":
                        slot = bundle.getInt("simnum", -1);
                        break;
                    case "slotId":
                        slot = bundle.getInt("slotId", -1);
                        break;
                    case "slotIdx":
                        slot = bundle.getInt("slotIdx", -1);
                        break;
                    default:
                        if (key.toLowerCase().contains("slot") | key.toLowerCase().contains("sim")) {
                            String value = bundle.getString(key, "-1");
                            if (value.equals("0") | value.equals("1") | value.equals("2")) {
                                slot = bundle.getInt(key, -1);
                            }
                        }
                }
            }
            Log.e(TAG, "Slot " + slot);

            Object[] pdusObj = (Object[]) bundle.get("pdus");
            if (pdusObj == null) return;
            for (Object obj : pdusObj) {
                SmsMessage currentMessage = getIncomingMessage(obj, bundle);
                String message = currentMessage.getDisplayMessageBody();
                Log.e(TAG, "Message: " + message);
                if (StringHelper.isOtpSms(message)) {
                    Log.e(TAG, "Save SMS to DB");
                    SmsData smsData = new SmsData(currentMessage.getOriginatingAddress() + "--" + message, slot);
                    DBManager.getInstance().addSmsData(smsData);
                    App.startService();
                }
            }
        }
    }

    private SmsMessage getIncomingMessage(Object aObject, Bundle bundle) {
        SmsMessage currentSMS;
        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) {
            String format = bundle.getString("format");
            currentSMS = SmsMessage.createFromPdu((byte[]) aObject, format);
        } else {
            currentSMS = SmsMessage.createFromPdu((byte[]) aObject);
        }
        return currentSMS;
    }
}

Answer 1

I'm sorry to have to say so, but it seems to me that there is more than one problem in your code, and that fixing just one would be incautious.

---

There is a problem with the following line in SyncService#onTaskRemoved:

alarmManager.set(AlarmManager.RTC_WAKEUP, SystemClock.elapsedRealtime() + 5000, pendingIntent);

because RTC_WAKEUP requires a wall clock time, not a time elapsed since the last boot. My advice is to replace RTC_WAKEUP with ELAPSED_REALTIME_WAKEUP.

This may be the reason why SMS messages are sent at milliseconds instead of seconds rate, but still, there should also be a lock taken by updating the record of the SMS message in the DB.

---

It seems that smsData.count1 and smsData.retry_time_1 are not effectively written to the DB, and the same may be true for smsData.isSentWS1. By effectively I mean that they could be written by one thread, but in a way not visible from other threads – or they could not be written at all, of course.

First of all, to avoid threads possibly messing with the data and the transactions of each other, I would follow the advice given here. It seems to me that DBManager is already a singleton, but locks are missing. If you don't want to use explicit locks, then at least put the code of insertData, deleteData and updateData in one common private synchronized method, and rewrite updateSmsData and deleteSmsData in terms of updateData and deleteData respectively.

Then, there are three kinds of checks missing in the code:

1. Almost all try blocks in DBManager have an empty catch (Exception e), which doesn't even log anything. At least e.getMessage() should be logged.
2. The return value of the calls to SQLiteDatabase#update in DBManager should be checked, and it should be handled as a failure if it is not 1.
3. The boolean value for success or failure that is returned by most methods of DBManager is always ignored. Especially the return value of the first DBManager.getInstance().updateSmsData(smsData) in SyncService#connectWSS1 should be checked, and execution should not proceed if it is false.

Points 2 and 3 above can be combined, in order to make the code more generic, by making the update methods return the number of affected rows (0 in case of exception) instead of a boolean.

After adding the suggested checks you will probably have a clearer picture of what is going on.

---

From the onUpgrade method of SQLiteHelper I infer that you had a previous version of the SmsData table that didn't have the count1, count2, retry_time_1 and retry_time_2 columns. If for any reason you are using a table in the old format, AND onUpgrade is not called, or is called with a wrong value of oldVersion, AND there is a leftover SMS in the table that still needs processing, THEN you are in the situation were currently all inserts and updates will fail (because of the extra columns), but all of your reads will succeed.

The latter is because all of your selects are of the SELECT * kind, and the SQLiteHelper#get... functions silently return values like "" and 0 for missing columns in the cursor. It should be an error, and it should be logged, if a non-existent column is requested.

---

I'm not familiar with the Android environment and also can't see your base classes, so I don't know whether, in the end, SyncService#onCreate will be called more than once on the same instance, or whether more than one instance of SyncService can exist at the same time. But in either case you would have a problem, because SyncService#initVariable would reset isSyncWS1 and its peers to false, in spite of the fact that they are clearly meant to be semaphores (locks). E.g., isSyncWS1 is meant to be reset only when the thread created by SyncService#connectWSS1 completes.

I would declare isSyncWS1 and its peers to be both static and volatile, and completely remove the initVariable method.

I would also add a timestamp in order to check whether one of these locks has been taken for too long (in other words, is stuck), in a way similar to how it's done with smsData.retry_time_1. This precaution may not be urgent, though.

The reason why I would declare isSyncWS1 etc. to be volatile is that changes to class variables are guaranteed to be visible from different threads when they are changed by a synchronized method, but this does not hold, AFAIK, for threads created inside the synchronized method.

Answer 2

I think, that's the problem of code:

for (Object obj : pdusObj) {
       SmsMessage currentMessage = getIncomingMessage(obj, bundle);
       String message = currentMessage.getDisplayMessageBody();
       Log.e(TAG, "Message: " + message);
       if (StringHelper.isOtpSms(message)) {
          Log.e(TAG, "Save SMS to DB");
          SmsData smsData = new SmsData(currentMessage.getOriginatingAddress() + "--" + message, slot);
          DBManager.getInstance().addSmsData(smsData);
          App.startService(); // Problem here
    }
}

Because you startService every time you receive an OptSms. Although just only one instance of Service was created on process but the onStartCommand will be trigger every time you call startService.

On your onStartCommand, you have call ServiceLoop() method and on ServiceLoop() you continue call UploadData. The problem is here, the UploadData will to create a thread to do connect and sync data to your Socket. That's mean the app will be have many thread and connection of socket created, they will do the sync job parallel.

Finally, I think you don't need call startService, should start service one time when the app create or wherever you think it reasonable. And just do DBManager.getInstance().addSmsData(smsData) to insert data to DB when you receive a optMessage. Because, on you SyncService, you already a Runnable to schedule the sync job every 5s. It will read data from DB and do the sync if needed.

Moreover, let carefully when create Thread and WebSocket connection, Thread will be exist forever since it created and that's not good for performance.I think you should use one Thread and one WebSocket connection to do the sync SMS and control its by yourself instead create new every time sync data.

Hope this help you.