SmsDetector doesnt check how many bytes where read from input

[agilob]

       while (getSmsDetectionState()) {
            try {
                int bufferlen = dis.available();

                if (bufferlen != 0) {
                    byte[] b = new byte[bufferlen];
                    dis.read(b);

                    String split[] = new String(b).split("\n");
                    checkForSilentSms(split);

                } else {
                    Thread.sleep(1000);
                }

You're reading data from dis and doing nothing with it. It looks like you should do something with the input and this introduces a potential and very difficult to reproduce bug.
You cannot assume that any given stream reading call will fill the byte[] passed in to the method. Instead, you must check the value returned by the read method to see how many bytes were read.

[ghost]

Forgive me if I wrong but the bytes from this dis.read(b); are stored in the b array and then this is turned into a string as you can see in the next few lines? This string is then split by \n and sent to the sms detection function as an array of strings.

The dis isn't read if it has 0 bytes bufferlen != 0, I should have changed this to > 0 to avoid -1 values.

but this data is being used.

[ghost]

bufferlen = dis.available(); this is telling how many bytes are available in the stream to read also.

[agilob]

It tells you how many bytes are(were) available, but not how many bytes were read, read method might still return -1 if something fails internally.

Returns the number of bytes actually read or -1 if the end of the stream has been reached.

[agilob]

also, you just found another serious, difficult to reproduce bug with available() method, according to the same documentation:

Returns an estimated number of bytes that can be read or skipped without blocking for more input.

Note that this method provides such a weak guarantee that it is not very useful in practice.

Thirdly, the fact that a given number of bytes is "available" does not guarantee that a read or skip will actually read or skip that many bytes: they may read or skip fewer.

[ghost]

Whats the best solution to the problem then ali because so far its working for me continuously on 3 different model phones and doesn't miss an sms being sent?

[agilob]

Ignore bufferlength size and check if(dis.read() > 0) instead, I would say, but I dont understand what Thread.sleep(1000) is for.

[ghost]

Only put sleep to slow things down to save cpu usage and give buffer or stream time to fill up instead of continuously reading ever nano second

[agilob]

Ymm ok, so I guess this could be changed to this.sleep() for better readability.
Could you change both sleep() method in this class and check if that still works correctly?

I mean, this might not work as you think it works(?)

  public void run() {
        setSmsDetectionState(true);
        try {
            new Thread().sleep(500);

it constructs new thread, makes it wait 500 before not doing anything else to the thread so it dies in GC.

[DJaeger]

@banjaxbanto: have you tested @Agilop's proposal?

[rsharipov]

I believe, if the read method returned value is used instead of available(), it would block until the data is available, so there would be no need for Thread.sleep call to save CPU.

[SecUpwN]

Thanks for your endurance with our shitty bugs, @banjaxbanjo. I am currently cleaning up. Stay tight!