concurrency driving me crazy

Discussion in 'Java' started by Andersen, Dec 10, 2005.

  1. Andersen

    Andersen Guest

    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, Dec 10, 2005
    #1
    1. Advertising

  2. Andersen

    Andersen Guest

    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).
     
    Andersen, Dec 10, 2005
    #2
    1. Advertising

  3. "Andersen" <> schreef in bericht
    news:439ab053$0$54073$...
    >
    > 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?
     
    Boudewijn Dijkstra, Dec 10, 2005
    #3
  4. Andersen

    Andersen Guest

    Boudewijn Dijkstra wrote:
    > 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);
    }
    }

    }

    }
     
    Andersen, Dec 10, 2005
    #4
  5. Andersen wrote:
    >
    > 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
    --
    Unemployed English Java programmer
    http://jroller.com/page/tackline/
     
    Thomas Hawtin, Dec 10, 2005
    #5
  6. "Andersen" <> schreef in bericht
    news:439af00d$0$54384$...
    > Boudewijn Dijkstra wrote:
    >> 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.
     
    Boudewijn Dijkstra, Dec 10, 2005
    #6
  7. Andersen

    Roedy Green Guest

    On Sat, 10 Dec 2005 11:39:05 +0100, Andersen
    <> 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.

    --
    Canadian Mind Products, Roedy Green.
    http://mindprod.com Java custom programming, consulting and coaching.
     
    Roedy Green, Dec 10, 2005
    #7
  8. Andersen

    Andersen Guest

    Boudewijn Dijkstra wrote:

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

    Absolutely sure!
     
    Andersen, Dec 11, 2005
    #8
  9. Andersen

    Andersen Guest

    Thomas Hawtin wrote:
    > 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.

    > > 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;
    >> 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;
    > }
    > }


    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.
     
    Andersen, Dec 11, 2005
    #9
  10. Andersen wrote:
    > Thomas Hawtin wrote:
    >
    >> 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.


    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
    --
    Unemployed English Java programmer
    http://jroller.com/page/tackline/
     
    Thomas Hawtin, Dec 11, 2005
    #10
  11. Thomas Hawtin wrote:
    > Andersen wrote:
    >
    >> Thomas Hawtin wrote:
    >>
    >>> 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.

    >
    >
    > 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
     
    Andrew McDonagh, Dec 11, 2005
    #11
  12. Andersen

    Andersen Guest

    Thomas Hawtin wrote:

    > 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?
     
    Andersen, Dec 12, 2005
    #12
  13. Andersen wrote:
    > Thomas Hawtin wrote:
    >
    >> 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
    --
    Unemployed English Java programmer
    http://jroller.com/page/tackline/
     
    Thomas Hawtin, Dec 12, 2005
    #13
  14. Andersen

    P.Hill Guest

    Thomas Hawtin wrote:
    > Andersen wrote:
    >
    >> Thomas Hawtin wrote:
    >>
    >>> 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
     
    P.Hill, Dec 16, 2005
    #14
  15. P.Hill wrote:
    > Thomas Hawtin wrote:
    >
    >> Andersen wrote:
    >>
    >>> Thomas Hawtin wrote:
    >>>
    >>>> 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


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


    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
    --
    Unemployed English Java programmer
    http://jroller.com/page/tackline/
     
    Thomas Hawtin, Dec 16, 2005
    #15
  16. Andersen

    Chris Uppal Guest

    Thomas Hawtin wrote:
    > 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
     
    Chris Uppal, Dec 17, 2005
    #16
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. William Gower

    ItemTemplate error driving me crazy

    William Gower, Apr 23, 2004, in forum: ASP .Net
    Replies:
    0
    Views:
    326
    William Gower
    Apr 23, 2004
  2. =?Utf-8?B?VGltOjouLg==?=

    Insert and Variables driving me crazy! Newbie...

    =?Utf-8?B?VGltOjouLg==?=, Dec 20, 2004, in forum: ASP .Net
    Replies:
    6
    Views:
    342
    =?Utf-8?B?VGltOjouLg==?=
    Dec 21, 2004
  3. Shapper
    Replies:
    0
    Views:
    338
    Shapper
    Apr 29, 2005
  4. Joe Blanchard via .NET 247
    Replies:
    1
    Views:
    771
    Eliyahu Goldin
    May 15, 2005
  5. Replies:
    3
    Views:
    437
    Kevin Spencer
    Jul 1, 2005
Loading...

Share This Page