Home | Contact Us | FAQ | Search & Site Map | Link to Us
Sign In | Join | Other 45 Sites in Network
HomeAnnouncementsWhite Papers
Discussion GroupsFirst AidDatabasesJavaBeansGUIJava 3DVirtual MachineCORBASecurityToolsGeneral
Java DirectoryOpen Source ProjectsSample Book ChaptersUser GroupsWeb Resources
Related Topics
Databases.NETMore Topics ...

Java Forum / General / December 2005

Tip: Looking for answers? Try searching our database.

concurrency driving me crazy

Thread view: 
Andersen - 10 Dec 2005 10:39 GMT
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 - 10 Dec 2005 10:44 GMT
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 - 10 Dec 2005 14:37 GMT
> I have two threads created sending messages to eachother (sockets). For
> every received message each thread a (not shared) counter, which is
[quoted text clipped - 11 lines]
> 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 - 10 Dec 2005 15:10 GMT
> 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 - 10 Dec 2005 21:31 GMT
> 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() {
[quoted text clipped - 13 lines]
>         }
>         };

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;
[quoted text clipped - 3 lines]
>         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
Signature

Unemployed English Java programmer
http://jroller.com/page/tackline/

Andersen - 11 Dec 2005 00:27 GMT
> 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.

>  >         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.

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).

>>     public boolean getBool() {
>>         boolean tmp;
[quoted text clipped - 15 lines]
>         }
>     }

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 - 11 Dec 2005 21:30 GMT
>> 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.

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
Signature

Unemployed English Java programmer
http://jroller.com/page/tackline/

Andrew McDonagh - 11 Dec 2005 22:49 GMT
>>> It's better in general to get in the habit of creating a Runnable
>>> passing it to Thread.
[quoted text clipped - 9 lines]
>
> 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=Go
ogle+Search&meta
=

Andrew
Andersen - 12 Dec 2005 06:16 GMT
> 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 - 12 Dec 2005 09:01 GMT
>> 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?

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
Signature

Unemployed English Java programmer
http://jroller.com/page/tackline/

P.Hill - 16 Dec 2005 06:48 GMT
>>> Count appears to be thread-safe (except for the initial assignment,
>>> which is quite common).

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.

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

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 - 16 Dec 2005 15:00 GMT
>>>> Count appears to be thread-safe (except for the initial assignment,
>>>> which is quite common).
[quoted text clipped - 4 lines]
>
> 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).

>>> Does the initial assignment matter? How does one guard itself against
>>> such problems?
>
> 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.

>> Usually it is assumed that objects will be passed to other threads in
>> a thread-safe manner.
[quoted text clipped - 3 lines]
>
> 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
Signature

Unemployed English Java programmer
http://jroller.com/page/tackline/

Chris Uppal - 17 Dec 2005 12:08 GMT
> 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
Boudewijn Dijkstra - 10 Dec 2005 22:01 GMT
>> A listing says more than a thousand words.  May the source be with us?
>
[quoted text clipped - 22 lines]
>
> [...]

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.
Andersen - 11 Dec 2005 00:23 GMT
> Are you sure the final println isn't happening at the *beginning*?

Absolutely sure!
Roedy Green - 10 Dec 2005 22:16 GMT
On Sat, 10 Dec 2005 11:39:05 +0100, Andersen
<andersen_800@hotmail.com> wrote, quoted or indirectly quoted someone
who said :

>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.

Signature

Canadian Mind Products, Roedy Green.
http://mindprod.com Java custom programming, consulting and coaching.



Free Magazines

Get these publications absolutely FREE for up to 12 months. There are no hidden fees and no obligation. Simply choose a title, complete the application form and submit it. Read more ...

Oracle MagazineNetwork ComputingComputer WorldBio-IT WorldeWeekInformation WeekInfosecurity
 
Sign In
Join
My Latest Posts
My Monitored Threads
My Blog
My Photo Gallery
My Profile
My Homepage

Start New Thread
Enable EMail Alerts
Rate this Thread



©2008 Advenet LLC   Privacy Policy - Terms of Use
This website includes both content owned or controlled by Advenet as well as content owned or controlled by third parties.