File reader and streamer component running in its own thread on Android

Question

In an Android project, I have a class whose job is to read lines from a file, and then pump those lines to a message handling thread, looping forever until told to stop. The main operation of this class (file reading + message sending) must be on its own separate thread, to run in parallel with whatever else the application is doing. Multi-threaded programming is not my strongest suit, so I'd like to reach out to those more knowledgeable than me in this area.

package net.bluetoothviewer;

import android.content.res.AssetManager;

import net.bluetoothviewer.util.AssetUtils;

import java.util.List;

public class MockLineByLineConnector implements DeviceConnector {

    public static final String SAMPLES_SUBDIR = "samples/line-by-line";

    private static final int SLEEP_MILLIS = 1000;

    private final MessageHandler messageHandler;
    private final AssetManager assets;
    private final String filename;

    private boolean running = false;

    public MockLineByLineConnector(MessageHandler messageHandler, AssetManager assets, String filename) {
        this.messageHandler = messageHandler;
        this.assets = assets;
        this.filename = filename;
    }

    @Override
    public synchronized void connect() {
        if (running) {
            return;
        }
        running = true;
        new Thread(new Runnable() {
            @Override
            public void run() {
                messageHandler.sendConnectingTo(filename);

                String mockFilePath = SAMPLES_SUBDIR + "/" + filename;
                List<String> lines = AssetUtils.readLinesFromStream(assets, mockFilePath);

                if (!lines.isEmpty()) {
                    loopLinesUntilStopped(lines);
                }

                messageHandler.sendConnectionLost();
            }

            private void loopLinesUntilStopped(List<String> lines) {
                messageHandler.sendConnectedTo(filename);

                while (running) {
                    for (String line : lines) {
                        if (!running) {
                            break;
                        }
                        messageHandler.sendLineRead(line);
                        try {
                            Thread.sleep(SLEEP_MILLIS);
                        } catch (InterruptedException e) {
                            // ok to swallow, this is our dedicated thread
                        }
                    }
                }
            }
        }).start();
    }

    @Override
    public synchronized void disconnect() {
        running = false;
    }

    @Override
    public void sendAsciiMessage(CharSequence chars) {
        // do nothing
    }
}

Do you spot issues with the way I work with the threads? The app seems to work perfectly well, but it's not exposed to high-concurrency scenarios that might trigger the problems caused by badly coded multi-threading.

Do I synchronize too much? Could I relax some of the synchronizations by making the running field volatile?

I'm aware that the AssetUtils.readLinesFromStream call within the synchronized region is suspicious, as it's an "alien method", but it has a trivial responsibility and I can trust it won't cause harm, so I think that can be acceptable in this case.

Using Thread and Runnable is a very low-level way of working with threads. Can I improve this code by using higher level techniques that are recommended in Android development or in Java in general?

Are there any other improvements you'd recommend?

The class is part of the Bluetooth Viewer Android app, an open source project. To play with it locally:

1. Clone the project from GitHub, and switch to tag v7 to get the same version as used in this question.
2. Build (in Android Studio, for example) and run it on a phone or emulator.
3. Enable mock devices: open the menu, select Settings, and check Enable mock devices.
4. Return to the main activity, click on the connect button, and select any of the mock devices.

Lines should start rolling on the screen, coming from this class under review.

Answer 1

One synchronise to much :

@Override
public synchronized void disconnect() {
    running = false;
}

You are declaring this synchronised. So if 2 threads are calling this method, the second one has to wait until the first one is finished.
Because you don't have an input variable, the output will always be the same, setting running false.
In this case there are no race conditions what could affect the outcome of this method, so I wouldn't make it synchronised.

private static final :

You are making a great use of the private static final fields.
What I'm not liking is the next step :

String mockFilePath = SAMPLES_SUBDIR + "/" + filename;

Why don't you put the "/" also in the SAMPLES_SUBDIR?

It's the only place where you use this field so it could be easily in the SAMPLES_SUBDIR.

Comments :

While I'm not the first to say you have to document your code, the following case is for me strange :

@Override
public void sendAsciiMessage(CharSequence chars) {
    // do nothing
}

You can do 2 things here.

• Throwing a UnsupportedOperationException so the next developer knows, this method you need to override is not supported here.
• Putting more comment then do nothing, why doesn't this has to do nothing? When I read that in a project that I have to take over, I'm thinking maybe TODO but why didn't he put TODO? So I'll be searching for the why and loosing maybe valuable time over a stupid issue.

All over :

Nice readable code, as an outstander, who doesn't know the things the other classes need to do I still understand what this class has to do.