Concurrency and multiple commands

[joelg236]

private int currentCommand;
private void runConcurrentCommands(final Vector<CommandBase> v) {
Thread thread = null;
for(currentCommand = 0; currentCommand < v.size(); currentCommand++) {
System.out.println("Current = "+currentCommand);
thread = new Thread(new Runnable() {
@Override
public void run() {
System.out.println("Running = "+currentCommand);
CommandBase.getInstance().run(v.get(currentCommand ));
}
});
thread.start();
}
try {
thread.join(1000);
} catch (InterruptedException ex) {

} catch (NullPointerException ex) {

}
}

Why does this give me the output of
run:
Sequential: team4334.code.commands.ExampleCommand@5b86d4c1
Run
Sequential: team4334.code.commands.ExampleCommand@70f9f9d8
Run
Sequential: team4334.code.features.Feature$1@2b820dda
Concurrent: team4334.code.commands.modes.ExampleMode@675b7986
Concurrent: team4334.code.commands.ExampleCommand@2687816d
Concurrent: team4334.code.commands.modes.ExampleMode@a422ede
Current = 0
Current = 1
Running = 1
Current = 2
Running = 2
Run
Run
Running = 3
Exception in thread "Thread-2" java.lang.ArrayIndexOutOfBoundsException: Array index out of range: 3
Sequential: team4334.code.commands.modes.ExampleMode@7ca83b8a
Run
at java.util.Vector.get(Vector.java:721)
at team4334.code.features.Feature$2.run(Feature.java: 113)
at java.lang.Thread.run(Thread.java:679)
BUILD SUCCESSFUL (total time: 3 seconds)

Is this a logic error or a concurrency error? In either case, what would be the best way to fix it?

I feel like the new Runnable is changing the value of currentCommand even before the for loop notices it is out of the range. If that is the case, how could I solve this problem? I have tried making the variable volatile (With the same result) and making the methods synchronized. I'm stumped.

[Mr. Lim]

Concurrency.

Your for loop is completing and incrementing currentCommand before your threads are actually starting.

Despite calling thread.start() - they don't actually have to start "right away."

This is why you also never see "Running = 0".

When your 1st spawned thread finally runs, the for loop has already completed, and currentCommand was incremented to 1 before the thread even ran...

A hack fix would be to add a thread.join(???) at the end of your for loop (after thread.start(), where ??? is just long enough to start your Command.

Not pretty, but it'll likely work.

Even better, pass currentCommand as a parameter to your threads... something like:
http://stackoverflow.com/questions/877096/how-can-i-pass-a-parameter-to-a-java-thread

Good luck!

[Mr. Lim]

Also, WPILib has support for concurrent Commands using CommandGroups.

It's documented in the WPILib cookbook.

Search for the AddParallel and AddSequential methods.

It's possible you might be trying to re-invent something that already works pretty well!

[joelg236]

It's possible you might be trying to re-invent something that already works pretty well!

Funny thing is that that is exactly what I'm trying to do. :D

A hack fix would be to add a thread.join(???) at the end of your for loop (after thread.start(), where ??? is just long enough to start your Command.

I feel like that is not a sufficiently consistent way of doing what I am trying to do. (As well as almost eliminating the purposes of doing it - that will either not wait long enough or make the commands run in sequence (and not concurrently))

My biggest issue is that when I tried to do this :
private void runConcurrentCommands(final Vector<CommandBase> v) {
currentCommand = 0;
Thread thread = null;
System.out.println("Current = "+currentCommand);
thread = new Thread(new Runnable() {
@Override
public void run() {
System.out.println("Running = "+currentCommand);
CommandBase.getInstance().run(v.get(currentCommand ));
currentCommand++;
}
});
thread.start();
}
it did not change the output.

To me, that would eliminate the problem by only incrementing the commandCount AFTER the command has run (And not incrementing it beforehand on the first run).

[joelg236]

Sorry about that obvious bug in the code up there. I added a loop to go through the commands, but still no result.

[joelg236]

Solved.

For the benefit of anyone that might have been curious as to the solution or are looking around for ways to do this - here it is.

private int currentCommands;
private synchronized void runConcurrentCommands(final Vector<CommandBase> v) {
try {
concurrentCommandsFinished = false;
Thread[] threads = new Thread[v.size()];
for(int x = 0; x < v.size(); x++) {
currentCommands = x;
threads[x] = new Thread(new Runnable() {
public final int commandNum = currentCommands;
@Override
public void run() {
System.out.println("Running "+commandNum);
//CommandBase.getInstance().run(v.get(commandNum));
}
});
}
for(int x = 0; x < threads.length; x++) {
threads[x].start();
}
for(int x = 0; x < threads.length; x++) {
while(threads[x].isAlive()) {
concurrentCommandsFinished = false;
}
}
concurrentCommandsFinished = true;
}catch(NullPointerException ex) {

}
}

private int lastThread = 0;
private boolean concurrentCommandsFinished = false;
private synchronized void waitForConcurrentCommands() throws InterruptedException {
while(!concurrentCommandsFinished) {
Thread.sleep(1);
}
}

public void enable() {
try {
this.interupted = false;
boolean adding = false;
Vector<CommandBase> concurrentCommands = new Vector<CommandBase>(1, 1);
for(int x = 0; x < commands.size(); x++) {
if(commandTypes.get(x).equals("Sequencial")) {
if(adding) {
runConcurrentCommands(concurrentCommands);
concurrentCommands = new Vector<CommandBase>(1, 1);
adding = false;
try {
waitForConcurrentCommands();
} catch (InterruptedException ex) {

}
}
System.out.println("Sequencial command "+x);
CommandBase.getInstance().run(commands.get(x));
}else if(commandTypes.get(x).equals("Concurrent")) {
adding = true;
concurrentCommands.add(commands.get(x));
}
}
if(adding) {
runConcurrentCommands(concurrentCommands);
adding = false;
}

}catch(NullPointerException ex) {

}
}

I created an array of threads - and did not run them until after they are all created. With this array, I needed a way to keep the variable of currentcommand in the state it was during creation of the object. That is why I have the
public final int commandNum = currentCommands;
in there.

In terms of running it all, I included the rest of the current code that I have. There is a portion that waits for an array of concurrent commands to finish before doing another sequencial command. Basically, this has almost all of the features of the CommandGroup class in WPI, while not using Scheduler or any WPI libraries (So it can be ported).

Again, thank you Mr.Lim for your help :)