1

Greets!

I am writing a simple server monitoring application in Java(JavaFX8). The current implementation is able to ping target machines one by one, and graph them onto a JavaFX LineChart. Each machine is a "Target" object, which is held in an ArrayList (Observable). My problem is the "one by one" part. The code to ping a target is a Callable that returns the ping. I, somehow, need to multithread the process so that I can ping the targets at least four at a time. Past attempts resulted in quirks such as four threads pinging the same target at the same time, resulting in a very pointless and processor intense redundancy. Heres my current loop...

public void beginPing() {
    ExecutorService exec = Executors.newCachedThreadPool();

    Runnable r = new Runnable() {
        @Override
        public void run() {
            while (true) {
                for (Target t : targets) {
                    String ping = null;
                    if (t.flagsProperty().get().contains("A")) {
                        try {
                            Callable c = new Pinger(t);
                            ping = c.call().toString();
                            switch (ping) {
                                case "TIME_OUT":
                                    for (XYChart.Series s : lineChart.getData()) {
                                        if (s.getName().equals(t.nameProperty().get())) {
                                            addToChart(s, cycle, 00.00);
                                        }
                                    }
                                    t.setStatus("TIME OUT");
                                    t.setLastrtt("TIME_OUT");
                                    t.setTimeouts(t.timeoutsProperty().get() + 1);
                                    logUtil.log(LogUtil.INFO, t.nameProperty().get() + " - timed out!");
                                    break;
                                case "UNKNOWN_HOST":
                                    t.setStatus("ERROR");
                                    t.setLastrtt("UNKNOWN HOST");
                                    logUtil.log(LogUtil.WARNING, t.nameProperty().get() + " - unknown host!");
                                    break;
                                case "UNREACHABLE":
                                    t.setStatus("ERROR");
                                    t.setLastrtt("UNREACHABLE HOST");
                                    logUtil.log(LogUtil.WARNING, t.nameProperty().get() + " - is unreachable!");
                                    break;
                                default:
                                    t.setLastrtt(ping);
                                    t.setStatus("ACTIVE");
                                    for (XYChart.Series s : lineChart.getData()) {
                                        if (s.getName().equals(t.nameProperty().get())) {
                                            addToChart(s, cycle, Double.valueOf(ping));
                                        }
                                    }
                                    break;
                            }
                        } catch (Exception e) {
                            logUtil.log(LogUtil.CRITICAL, e.getMessage() + ", "+ e.getCause());
                            e.printStackTrace();
                        }
                    }
                }
                cycle++;
                rangeChart(cycle);
                updateInfo();
            }
        }
    };
    exec.execute(r);
}
Linas Martusevicius
  • 81
  • 1
  • 9
  • I think you have to make 4 separate threads for that – Muhammed Refaat Nov 10 '14 at 07:48
  • 1
    I do, but the question is that I need to do that in a way, so the 4 separate threads don't iterate over the same Target t in a cycle, otherwise it would just four threads doing the same thing at the same time, which could lead to ConcurrentModificationExceptions and pointless redundancy. – Linas Martusevicius Nov 10 '14 at 08:55
  • There is some sample code for multithreading calls over an array in JavaFX in the answer to the kind of misnamed question: [How to reset progress indicator between tasks in JavaFX2?](http://stackoverflow.com/questions/16368793/how-to-reset-progress-indicator-between-tasks-in-javafx2) – jewelsea Nov 11 '14 at 01:12

2 Answers2

1

My impression is that you misuse your Callable class Pinger like a regular class, although it is only an interface that does not implement any multithreading services. The thing you want to do should look more like this:

//init
Future<String> futures = new Future[targets.length];
String results = new String[targets.length];
ExecutorService service =  Executors.newCachedThreadPool();

//start Threads
for (int i = 0; i<targets.length; i++){      
    Pinger pinger= new Pinger(targets[i]);
    future[i] = service.submit(pinger);
}

//wait for Threads to finish and get results
for(int i = 0; i<futures.length; i++)
    results[i] = futures[i].get()

Your Pinger should look like this:

public class Pinger implements Callable<String>{
    Pinger(Target target){ ... }
    public String call(){ ... }
}

Here you find a fully implemented Example for Callables. In your code you only submit one Runnable to the ExecutorService, so there will be only two threads (Main and your Runnable). You never call the method call(), this is done by the ExecutorService. Compare this to the Runnable Interface you have to execute the Thread calling start or submitting it to a ExecutorService instead of calling run(); You use the Future that is returned during the submit(). Just try to understand the concept of Callable and then you will be able to write everything you want. ;-)

Rick
  • 2,080
  • 14
  • 27
  • I do call callable.call(), i just immediately .toString() it. At the top of the for loop, just before the switch. The runnable is there to keep that JavaFX UI thread from becoming unresponsive. And Pinger already implements Callable. – Linas Martusevicius Nov 10 '14 at 08:30
  • Exactly this is your problem. By calling the method call() there is no multithreading done, you just have sequential behaviour. You need to submit each Callable to the ExecutorService – Rick Nov 10 '14 at 10:53
  • Perhaps I should clarify; I know I need to call the Callable Pinger on the Target(s) simultaneously. The problem is that I somehow need to do that and update the Target data as soon as the ping responds(or times out). Using Futures, and your example, would mean that I would have to wait for all of the responses, and then update the Target variables all in a single operation. Also, I somehow need to ensure that the different threads in the ExecutorService do not ping the same Target at the same time (or one thread after another, even). In essence, i need the flow of events to look something... – Linas Martusevicius Nov 10 '14 at 11:20
  • ...like this. [Thread 1, Target 1][Thread 2, Target 2][Thread 1, Target 3][Thread 2, Target 4]...etc. Using your implementation, it would look like this: [Thread 1, Target 1][Thread 2, Target 1][Thread 1, Target 2][Thread 2, Target 2] (given the threadpool is limited to 2 threads). Id have unnecessary pings and the Target list iteration cycle would be just as long. – Linas Martusevicius Nov 10 '14 at 11:24
  • 1
    @LinasMartusevicius hmm .. how about keeping the targets in some type of thread-safe data structure, let each thread take (literally, by removing) a target from the top of the data, process, then re-add at its end? The taking can't be really simultaneous, but might be near enough? – kleopatra Nov 10 '14 at 11:30
  • @kleopatra sounds simple, the only thing id need to keep the free threads from using the same target would be to add the finished ones to the end. Might give it a shot! If anyone cares to point out why that would be inefficient, is welcome to do so, but I think this might be the perfect solution for a project of my scale. – Linas Martusevicius Nov 10 '14 at 11:41
  • Ok now I understand your problem. To make this clear with Callables. You should understand them as jobs and not as Threads, because you use a ExecutorService. This means every core of your cpu gets a job/callable and as soon as one job/callable is finished the thread/core gets another job. Since you give every Callable a different target you do not get redundant pings. – Rick Nov 10 '14 at 11:48
  • The challenge is that you need to constantly update your data. Therefore I would not use Callable, at least no the callable mechanism. My first though on this is that you pass the reference of your GUI to each job/callable/runnable and as soon as they calculated their result you update the GUI. – Rick Nov 10 '14 at 11:49
  • 1
    @Zephro I know they are jobs and not threads, that's why I create one for every Target in the while(true){for (Target t:targets){Callable c = new Pinger(t);//etc]}} The runnable was just a wrapper(if you will) to keep the ui thread from freezing while im looking for a better implementation ;) but thanks for the input, the Future solution would have been useful under slightly different circumstances! ;) Will answer my own question once I figure out and streamline kleopatra's solution. – Linas Martusevicius Nov 10 '14 at 12:15
0

So, heres the current working implementation...

public void beginPing() {
    safeTargets = new ArrayList<>(); //thread-safe collection
    for (Target t : targets) {
        safeTargets.add(t);
    }
    safeTargets = Collections.synchronizedList(targets);

    exec = Executors.newCachedThreadPool();

    for (int i = 0; i < 4; i++) { //number of threads
        exec.execute(new Runnable() {
            @Override
            public void run() {
                while (true) {
                    for (Target t : safeTargets) {
                        String ping = null;
                        if (t.isActive() && !t.isIsBeingPinged()) { //checks if target is already being pinged by another thread and if it flagged as active and wishes to be pinged.
                            t.setIsBeingPinged(true);
                            t.setPinged(t.getPinged() + 1); //just to see how many times it has been pinged
                            t.setStatus("PINGING");
                            try {
                                Callable c = new Pinger(t);
                                ping = c.call().toString();
                                switch (ping) {
                                    case "TIME_OUT":
                                        t.setStatus("TIME OUT");
                                        t.setLastrtt("TIME_OUT");
                                        t.setTimeouts(t.timeoutsProperty().get() + 1);
                                        logUtil.log(LogUtil.INFO, t.nameProperty().get() + " - timed out!");
                                        t.setIsBeingPinged(false);
                                        break;
                                    case "UNKNOWN_HOST":
                                        t.setStatus("ERROR");
                                        t.setLastrtt("UNKNOWN HOST");
                                        logUtil.log(LogUtil.WARNING, t.nameProperty().get() + " - unknown host!");
                                        t.setIsBeingPinged(false);
                                        break;
                                    case "UNREACHABLE":
                                        t.setStatus("ERROR");
                                        t.setLastrtt("UNREACHABLE HOST");
                                        logUtil.log(LogUtil.WARNING, t.nameProperty().get() + " - is unreachable!");
                                        t.setIsBeingPinged(false);
                                        break;
                                    default:
                                        t.setLastrtt(ping);
                                        t.setStatus("ACTIVE");
                                        t.setIsBeingPinged(false);
                                        break;
                                }
                                System.out.println("C=" + t.getPinged() + " - " + t.nameProperty().get());
                            } catch (Exception e) {
                                logUtil.log(LogUtil.CRITICAL, e.getMessage() + ", " + e.getCause());
                                e.printStackTrace();
                            }
                        }
                    }
                }
            }
        });
    }
}

I had to get rid of the immediate addition to the chart after all.

  • The Target list objects get added to a thread-safe synchronizedList (as suggested by kleopatra).
  • A boolean variable was added to the Target model to determine if it is currently being pinged by one of the threads. (t.isIsBeingPinged())
  • The data gets added to the chart using a new Runnable in the same pool, which iterates the target list and adds the last RTT to the chart every second, to avoid Targets with higher pings from falling behind on the chart.

Thanks for the very quick responses!

Linas Martusevicius
  • 81
  • 1
  • 9