concurrency driving me crazy

[Andersen]

I have two threads created sending messages to eachother (sockets). For
every received message each thread a (not shared) counter, which is
increased for each received message. The counters are output to screen
all the time.

I start both threads. I join on each of the threads (wait for them to
finish). Then I poll each counter to see how many messages was sent.

Problem:
On the screen, the counters are correct (showing 1000), but when I type
an assertion, I get a failure saying that the counters are way lower
than 1000.

Why is this? Does it have to do with java's memory model? I seem to be
accessing old values? If I do a System.out.println(thread1.getCount())
in the parent thread, it all suddenly works.

ps. this is a JUnit testcase.

 

[Andersen]

I got another weird behaviour for the program.

If I put a System.out.println("whatever") after I join the threads,
nothing shows! Everything thereafter will work properly, i.e. the
counter reads will be correct. But one println will basically not show
(any other print lines will show).

 

[Boudewijn Dijkstra]

I have two threads created sending messages to eachother (sockets). For
every received message each thread a (not shared) counter, which is
increased for each received message. The counters are output to screen all
the time.

I start both threads. I join on each of the threads (wait for them to
finish). Then I poll each counter to see how many messages was sent.

Problem:
On the screen, the counters are correct (showing 1000), but when I type an
assertion, I get a failure saying that the counters are way lower than 1000.

Why is this? Does it have to do with java's memory model? I seem to be
accessing old values? If I do a System.out.println(thread1.getCount()) in
the parent thread, it all suddenly works.

A listing says more than a thousand words. May the source be with us?

 

[Andersen]

A listing says more than a thousand words. May the source be with us?

Certainly. Certain parts will be missing, but here it is. Search for ***
to find the place where the problem occurs.

public class RouteAsyncTest extends TestCase {
private final int START_PORT = 4440;
private DHTImpl node1 = null;
private DHTImpl node2 = null;
private ConnectionManager cm1 = null;
private ConnectionManager cm2 = null;
private AppHandler app1 = null;
private AppHandler app2 = null;
private final long ROUTECOUNT = 1000;

public void testManyPing() {
Thread t1 = new Thread() {
public void run() {
for (long cc = 0; cc<ROUTECOUNT; cc++) {
node1.routeAsync(10, new PingMsg(cc).flatten());
Thread.yield();
}
}
};

Thread t2 = new Thread() {
public void run() {
for (long cc = 0; cc<ROUTECOUNT; cc++) {
node2.routeAsync(0, new PingMsg(cc).flatten());
Thread.yield();
}
}
};

t1.start();
t2.start();
try {
t1.join();
t2.join();
} catch (Exception ex) {
ex.printStackTrace();
}

// *** This is were I get the fishy behaviour.

System.out.println();
long c1 = app1.getCount();
long c2 = app2.getCount();
assertEquals(c1, c2);
assertEquals(c1, ROUTECOUNT);
}

public void setUp() {
try {
cm1 = ConnectionManager.newInstance( (int) START_PORT); // Static
factory only used for testing, should use getInstance (singleton)
cm2 = ConnectionManager.newInstance( (int) START_PORT+1); // Static
factory only used for testing, should use getInstance (singleton)

app1 = new AppHandler("node1");
app2 = new AppHandler("node2");
node1 = new DHTImpl(cm1, app1);
node2 = new DHTImpl(cm1, app2);

node1.create();

node2.join( node1 );

}
catch (Exception ex) {
ex.printStackTrace();
}
}

public void tearDown() {
sleep(500);
node1.leave(); // causes the node to gracefully leave
sleep(500);
node2.leave(); // causes the node to gracefully leave
sleep(500);

cm1.end(); // Closes down all the sockets and threads
cm2.end(); // Closes down all the sockets and threads
}

public void sleep(int msec) {
try {
Thread.sleep(msec);
}
catch (Exception ex)
{
ex.printStackTrace();
}
}

public static class AppHandler implements DHTCallback {
private String name = "";
private Boolean bol = Boolean.FALSE;
private long count = 0;

public synchronized long getCount() { return count; }
public synchronized void incCount() { count++; }

public boolean getBool() {
boolean tmp;
synchronized (bol) {
tmp = bol.booleanValue();
}
return tmp;
}

public void setBool(boolean b) {
synchronized (bol) {
bol = Boolean.valueOf(b);
}
}

public void dhtRouteCallbackAsync(long identifier, DKSMessage payload) {
if (payload instanceof PingMsg) {
setBool(true);
incCount();
PingMsg p = (PingMsg) payload;
System.out.println(name+": got ping("+p.getCount()+")"+"
total("+getCount()+")");
} else {
System.err.println("Error parsting : "+payload);
}
}

}

}

 

[Thomas Hawtin]

Certainly. Certain parts will be missing, but here it is. Search for ***
to find the place where the problem occurs.

Thread t1 = new Thread() {
public void run() {
for (long cc = 0; cc<ROUTECOUNT; cc++) {
node1.routeAsync(10, new PingMsg(cc).flatten());
Thread.yield();
}
}
};

Thread t2 = new Thread() {
public void run() {
for (long cc = 0; cc<ROUTECOUNT; cc++) {
node2.routeAsync(0, new PingMsg(cc).flatten());
Thread.yield();
}
}
};

It's better in general to get in the habit of creating a Runnable
passing it to Thread.

The copy and paste here is not good.

} catch (Exception ex) {
ex.printStackTrace();
}

throw new Error(ex); would work better here.

cm2 = ConnectionManager.newInstance( (int) START_PORT+1); //

Static factory only used for testing, should use getInstance (singleton)

You never use this variable, as far as I can see.

private Boolean bol = Boolean.FALSE;
private long count = 0;

public synchronized long getCount() { return count; }
public synchronized void incCount() { count++; }

Count appears to be thread-safe (except for the initial assignment,
which is quite common).

public boolean getBool() {
boolean tmp;
synchronized (bol) {
tmp = bol.booleanValue();
}
return tmp;
}

This does not make any sense. You are not synchronising on a permanent
feature of the class. Instead, write something like:

private boolean flag = false;
private static class FlagLock { }
private final Object flagLock = new FlagLock();
...
public boolean isFlag() {
synchronized (flagLock) {
return flag;
}
}

public void dhtRouteCallbackAsync(long identifier, DKSMessage
payload) {
if (payload instanceof PingMsg) {

I assume from your other message, that this does indirectly get called
with that condition true. Perhaps if you simplified things to the point
where a simple example was compilable, it would become clearer.

Tom Hawtin

 

[Boudewijn Dijkstra]

A listing says more than a thousand words. May the source be with us?

Certainly. Certain parts will be missing, but here it is. Search for *** to
find the place where the problem occurs.

[...]

t1.start();
t2.start();
try {
t1.join();
t2.join();
} catch (Exception ex) {
ex.printStackTrace();
}

// *** This is were I get the fishy behaviour.

System.out.println();
long c1 = app1.getCount();
long c2 = app2.getCount();
assertEquals(c1, c2);
assertEquals(c1, ROUTECOUNT);
}

[...]

Are you sure the final println isn't happening at the *beginning*?

When sending messages over sockets, they may pass through several threads
and/or processes before arriving at the destination. Because the messages are
passed asynchronously (=without waiting for it to get delivered), t1 and t2
will probably finish very quickly. So when the counts are queried, some
messages will still be queued somewhere and the assertion will fail.

 

[Roedy Green]

On the screen, the counters are correct (showing 1000), but when I type
an assertion, I get a failure saying that the counters are way lower
than 1000.

Are these fields volatile -- changed by two different threads outside
a synchronized block? If so declare them so. Otherwise you fool the
optimiser. You are getting register copies rather than ram copies.

 

[Andersen]

Are you sure the final println isn't happening at the *beginning*?

Absolutely sure!

 

[Andersen]

Andersen wrote:

It's better in general to get in the habit of creating a Runnable
passing it to Thread.

Do not see why, except if I need the inheritance for something else.

throw new Error(ex); would work better here.

You are right.

Static factory only used for testing, should use getInstance (singleton)

You never use this variable, as far as I can see.

True, that is a bug, but does not affect things. Thanks!

Count appears to be thread-safe (except for the initial assignment,
which is quite common).


This does not make any sense. You are not synchronising on a permanent
feature of the class. Instead, write something like:

private boolean flag = false;
private static class FlagLock { }
private final Object flagLock = new FlagLock();
...
public boolean isFlag() {
synchronized (flagLock) {
return flag;
}
}

You are right.

I assume from your other message, that this does indirectly get called
with that condition true. Perhaps if you simplified things to the point
where a simple example was compilable, it would become clearer.

I'll do that and get back.

 

[Thomas Hawtin]

Do not see why, except if I need the inheritance for something else.

I have seen commercial software shipped without thread-pooling for
exactly that abuse.

It is criminally bad coding to extend a class when there is no need to.
It ties two unrelated concepts and makes the new class more complicated
and less flexible.

Tom Hawtin

 

[Andrew McDonagh]

I have seen commercial software shipped without thread-pooling for
exactly that abuse.

It is criminally bad coding to extend a class when there is no need to.
It ties two unrelated concepts and makes the new class more complicated
and less flexible.

Tom Hawtin

yep - and inherently less Unit testable.

Anderson, the use of inheritance is when there is a 'is a' relationship,
not because is an easy way to get code reuse.

We aren't creating a new type of Thread class - all Thread classes do is
handle running something in a different thread.

Deriving from Thread just to do the custom behavior is wrong on so many
(OO) scales.


Always 'favor delegation over inheritance' (a nice google-able term).


http://www.google.co.uk/search?hl=en&q=favor+delegation+over+inheritance&btnG=Google+Search&meta=

Andrew

 

[Andersen]

Count appears to be thread-safe (except for the initial assignment,
which is quite common).

Does the initial assignment matter? How does one guard itself against
such problems?

 

[Thomas Hawtin]

Does the initial assignment matter? How does one guard itself against
such problems?

Usually it is assumed that objects will be passed to other threads in a
thread-safe manner.

There is a theoretical possibility that the pre-initialised value (0)
will be seen or the initialisation value will overwrite an updated value.

You can initialise the value within a synchronised block, but in
practice no-one bothers.

Tom Hawtin

 

[P.Hill]

I'm not sure what your concern it on this assignment, since
the initial assignment is
(1) is to the default initial value.

http://java.sun.com/docs/books/jls/third_edition/html/typesValues.html#4.12.5
"For type int, the default value is zero, that is, 0."

(2) in the initialization for the member

So I can't see where there could be any uninitialized object leaking out
even if the initial value was something other than 0.

Now, theoretically, I could invent various perverse cases using
initializer blocks that would expose an incomplete object. Alternatively
there is the case that seems easier to fall into where a super class
registers an object before the subclass is finished initializing it, but
I didn't see any such problems in the OPs code.

Never place an object in ANY data structure which might be accessible
to another thread anywhere until after ALL constructors have completed.
An assignment of the result of a new does not violate this rule of thumb.

Usually it is assumed that objects will be passed to other threads in a
thread-safe manner.

There is a theoretical possibility that the pre-initialised value (0)
will be seen or the initialisation value will overwrite an updated value.

Really? Not in the OPs case; please explain.

-Paul

 

[Thomas Hawtin]

I'm not sure what your concern it on this assignment, since
the initial assignment is
(1) is to the default initial value.

http://java.sun.com/docs/books/jls/third_edition/html/typesValues.html#4.12.5

Yes, you are safe with the initial value. You are even safe (from 1.5)
if count were declared final.

"For type int, the default value is zero, that is, 0."

The initialised value is 0, yes. However, there is still the question of
the assignment in this code. *Theoretically* increments could happen
before this assignment. The assignment would then trash the incremented
value.

(2) in the initialization for the member

So I can't see where there could be any uninitialized object leaking out
even if the initial value was something other than 0.

Then you are failing to fully grasp the nature of threading (in the
presence of optimisations).

Now, theoretically, I could invent various perverse cases using
initializer blocks that would expose an incomplete object. Alternatively
there is the case that seems easier to fall into where a super class
registers an object before the subclass is finished initializing it, but
I didn't see any such problems in the OPs code.

Not all of the original posters code was threaded. It is a public class.

But most importantly, that's all irrelevant. The Java memory model does
not guarantee anything like sequentially consistency. This isn't just
important on multi-threaded hardware, but on JVMs that optimise code
(i.e. pretty much any worthwhile production JVM).

Never place an object in ANY data structure which might be accessible
to another thread anywhere until after ALL constructors have completed.

Good idea.

An assignment of the result of a new does not violate this rule of thumb.

Not really true. You want a "happens-before" edge between construction
and use. Normally this just means proper synchronisation. Use of final
field semantics is another approach.

Really? Not in the OPs case; please explain.

Not in the (partial) code posted. But the class is public, so anything
could go on, even in code not yet written.

As I say, the usual unstated (and probably unknown by the author)
assumption is that the object is distributed to other threads in a
thread-safe manner. An obscure, nasty corner case is that if an object
with a finaliser is dropped immediately after construction (technically
after the Object constructor returns to the next subclass constructor),
then it is not transferred thread-safely to the finaliser thread it runs on.

Tom Hawtin

 

[Chris Uppal]

An obscure, nasty corner case is that if an object
with a finaliser is dropped immediately after construction (technically
after the Object constructor returns to the next subclass constructor),
then it is not transferred thread-safely to the finaliser thread it runs
on.

Do you care to expand on that ?

I'm finding it hard to see how there's a problem. In the first case I can't
think of any way that a reference to an object /can/ be dropped before all its
constructors have completed (either by a normal return or a thrown exception).
In the second, I don't see how that, if it happened, would affect how the
object is put on the finalisation queue. And if enqueuing doesn't involve (the
equivalent of) crossing a synchronisation barrier, then it's more than a "nasty
corner case", it's a sodding great hole in the implementation !

-- chris