Synchronized everything!



  • So I updated our code to use a newer version of another internal team's mock service package this afternoon. (It provides a mock Java interface for their web service, for use in unit tests.) The update made one of our unit tests freeze when building from the command line (but almost never when running that same unit test in Eclipse); eventaully I stopped bashing my head against the wall and pulled up the commit log for their package in source control. This is what I saw:

    Revision 123456 (someuser): New synchronization stuff; synchronized everything!

    I wish I could tell you that he wasn't being literal.

    public synchronized String getSomeValue() {
        return null;  // this isn't relevant to my story, but this function was actually present in the code
    }
    
    public synchronized SomeOtherThing getOtherThing() {
        return someOtherMemberVariable;
    }
    
    public synchronized UnrelatedSummaryObject getSummary() {
        // some code that generates an UnrelatedSummary object,
        // doesn't ever touch someOtherMemberVariable
       return unrelatedSummaryObject;
    }

    You have probably already guessed what happened... deadlock! A common use-case of the (actual) service is for one thread to call getOtherThing() as it terminates, while another thread loops on getSummary() until the summary object indicates that some third thing has occurred. Turns out that doesn't work so well when both threads use the same mock service object and the writer of the class was overzealous with "synchronized"...

    I'm still mad this wasted my whole afternoon.



  • Solution: find the guilty developer and force the concurrency chapter of a good Java book down his throat



  • @dtech said:

    Solution: find the guilty developer and force the concurrency chapter of a good Java book down his throat

    Better solution: get a few developer friends and concurrently beat him with multiple copies of the good Java book to illustrate proper parallel processing.

     



  • @snoofle said:

    Better solution: get a few developer friends and concurrently beat him with multiple copies of the good Java book to illustrate proper parallel processing.

     

    Would that be round-robin or truly parallel?


  • 🚽 Regular

    @snoofle said:

    Better solution: get a few developer friends and concurrently beat him with multiple copies of the good Java book to illustrate proper parallel processing.
     

    Even better solution: Get a few developers to force him to buy a round of drinks for breaking unit tests which should have been passing before he even committed his code.



  • Yeah, we're all baffled as to how his own unit tests pass. My only guess is that the tests are far too simple...



  •  @dhromed said:

    @snoofle said:

    Better solution: get a few developer friends and concurrently beat him with multiple copies of the good Java book to illustrate proper parallel processing.

     

    Would that be round-robin or truly parallel?

    A good question, because concurrency is not the same as parallelism.


  •  I've long wished Java had never allowed the synchronized keyword as a method modifier.  People who know nothing about concurrency just slap that on there as if everything's thread safe now.  If only synchronized blocks that were explicit in the monitor were allowed at least you'd be forced to think about what you put in there.



  • There are good Java books?



  • @Heron said:

    Yeah, we're all baffled as to how his own unit tests pass. My only guess is that the tests are far too simple...

    My guess is that it's a bug in multithreaded code. It could be that he only ran the tests in Eclipse (that you said do pass sometimes). Or maybe he has a sigle-core processor while you have a multi-core one. Or maybe the other way aroud. Maybe he has a different version of Java (I mean, I don't bother to update that POS). Different OS patches? Different version of the testing framework? Running programs? Disk fragmentation? Phase of the moon?

    What I'm trying to say is that multithreaded code is hard to develop, debug, and test.



  • @Eternal Density said:

    There are good Java books?
     

    Good for beating people, yes. Some of them are quite heavy.



  • What's the problem? His comment was completely accurate: he synchronized everything. He never said he made it work correctly.



  • @SlyEcho said:

    My guess is that it's a bug in multithreaded code.

    I'm pretty sure I already explained exactly what caused the deadlock :P The WTF there is not that a deadlock happened at all, but that the deadlock happened on a common use case which they should have been testing in their own unit test suite. (And if they are, we're baffled how their unit tests pass.)

    What I'm trying to say is that multithreaded code is hard to develop, debug, and test.

    TRWTF is not that multithreading was too hard for this developer, it's that a) he thought it was a good idea to synchronize everything, and b) the "synchronize everything!" commit got through the peer code review process.

    @toth said:

    What's the problem? His comment was completely accurate: he synchronized everything. He never said he made it work correctly.

    The accuracy of one's commit comment does not make one's code immune to WTFs...



  • @Heron said:

    I'm pretty sure I already explained exactly what caused the deadlock :P

    Did you explain it exactly?-) I don't get how this causes deadlocks:

        Object member; // this is set somewhere
        
        public synchronized Object getMember() { return member; }
        public synchronized Object getFoo() { 
          // code to generate foo, NOT RELATED TO member nor to getMember()
          return new Object(); 
        }
    

    Could you provide code example generating that deadlock?



  • @dalby said:

    Could you provide code example generating that deadlock?

    From my original post:

    @Heron said:

    A common use-case of the (actual) service is for one thread to call getOtherThing() as it terminates, while another thread loops on getSummary() until the summary object indicates that some third thing has occurred.

    So, using your two example functions, and assuming the synchronized object is an instance named "dude", we have two threads:

    // Thread A
    while(true) {
        dude.getMember();
    }
    
    // Thread B
    dude.getFoo();
    

    Thread A's loop on getMember() will essentially permanently hold the lock on "dude", so thread B will never enter getFoo().



  • Ahem, sorry, Heron, your code is nowhere near deadlock situations.

    a) Thread A is not permanently holding the lock, it is rather repeatedly acquiring and releasing it.

    b) Thread B just waits until thread A released it once, gets in between and does its stuff, and then leaves the rest of the time back to A.

    Deadlock situations need a slight bit more complexity, but I can't be bothered now as it is already past midnight here.



  • "Nowhere near"? The simplified example above may not deadlock; in the real code I'm working with, the while(true) loop is calling a synchronized getter which builds a large summary object, and meanwhile the other thread's single synchronized call to getMember() (which just returns a simple member variable) never enters the function. I watched it happen in the debugger.

    I'd say my simplified example is, well, simplified, but it's silly to say it's "nowhere near" deadlock. All it takes is the scheduler always switching tasks while thread A is in the middle of the getter it calls. It's really not much more code than what's in the simplified example above.

    At any rate, this is beside the point. TRWTF of this situation is the "synchronize everything!" commit comment; clearly, the programmer in question did not understand what "synchronized" does. Adding "synchronized" to every method on a class is generally [i]not[/i] the right thing to do.

    Edit: You'll also note that I said "it essentially permanently holds the lock"; it may as well be permanent if every time you release it, you instantly grab it again.



  • Wow—really? Even if Java doesn't enforce any sort of order on its locks, I'd expect the blocked thread to luck out eventually and proceed.



  • @Enterprise Architect said:

    Wow—really? Even if Java doesn't enforce any sort of order on its locks, I'd expect the blocked thread to luck out eventually and proceed.

    That would be nice, wouldn't it? I let it run overnight, and it was still deadlocked.



  • Confirmed:

    public class Demo {
    private int doucheRunCount = 0;

    public static void main(String args[ ]) {
    	new Demo().run();
    }
    
    public void run() {
    	Thread douche = new Thread() {
    		public void run() {
    			while(true) {
    				synchronized(Demo.this) {
    					try {
    						sleep(5000); // What a douche
    					} catch (InterruptedException e) {
    						// Who cares?!
    					}
    					++doucheRunCount;
    				}
    			}
    		}
    	};
    	Thread reader = new Thread() {
    		public void run() {
    			while(true) {
    				synchronized(Demo.this) {
    					System.out.println(
    							String.format("Douche has run %d times",
    									doucheRunCount));
    				}
    			}
    		}
    	};
    	reader.start();
    	douche.start();
    }
    

    }

    This, however, does not deadlock, though I can't prove it's guaranteed not to:
    #include <pthread.h>
    #include <stdio.h>

    static int douche_run_count;
    static pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;

    static void *reader_thread(void *data) {
    for(;;) {
    pthread_mutex_lock(&mutex);
    printf("Douche has run %d times\n", douche_run_count);
    pthread_mutex_unlock(&mutex);
    }
    }

    int main(int argc, char *argv[]) {
    pthread_t thread;
    pthread_create(&thread, NULL, &reader_thread, NULL);
    for(;;) {
    pthread_mutex_lock(&mutex);
    sleep(5); // What a douche
    ++douche_run_count;
    pthread_mutex_unlock(&mutex);
    }
    }

    TRWTF is the C is shorter than the Java…



  • @Enterprise Architect said:

    Confirmed:
    public class Demo {
    private int doucheRunCount = 0;

    public static void main(String args[ ]) {
    	new Demo().run();
    }
    
    public void run() {
    	Thread douche = new Thread() {
    		public void run() {
    			while(true) {
    				synchronized(Demo.this) {
    					try {
    						sleep(5000); // What a douche
    					} catch (InterruptedException e) {
    						// Who cares?!
    					}
    					++doucheRunCount;
    				}
    			}
    		}
    	};
    	Thread reader = new Thread() {
    		public void run() {
    			while(true) {
    				synchronized(Demo.this) {
    					System.out.println(
    							String.format("Douche has run %d times",
    									doucheRunCount));
    				}
    			}
    		}
    	};
    	reader.start();
    	douche.start();
    }
    

    }

    This is NOT per my request at all. Try again using those synchronized methods, I still think it can't deadlock because the example was oversimplified.

    Another thing is that your example does not deadlock at all if you change that 5000 ms to 1 ms but it runs perfectly... which implies that your example does NOT deadlock. 5 seconds grab to that lock is just plain stupid but it wasn't asked here.

    TRWTF is the C is shorter than the Java…

    Why? And why you are using String.format() instead of System.out.format()? Btw, Java has for (;;) too.



  • @Heron said:

    I'd say my simplified example is, well, simplified, but it's silly to say it's "nowhere near" deadlock. All it takes is the scheduler always switching tasks while thread A is in the middle of the getter it calls. It's really not much more code than what's in the simplified example above.

    It's still not a deadlock — a deadlock is when both threads are permanently blocked, while in your case at least one thread progresses.



  • @Spectre said:

    It's still not a deadlock — a deadlock is when both threads are permanently blocked, while in your case at least one thread progresses.

    Ok. What term do you use when your unit test never finishes because a thread can't get a lock that another thread is hogging all to itself?



  • @Heron said:

    Ok. What term do you use when your unit test never finishes because a thread can't get a lock that another thread is hogging all to itself?
    A hang.



  • @dalby said:

    This is NOT per my request at all. Try again using those synchronized methods, I still think it can't deadlock because the example was oversimplified.

    Another thing is that your example does not deadlock at all if you change that 5000 ms to 1 ms but it runs perfectly... which implies that your example does NOT deadlock. 5 seconds grab to that lock is just plain stupid but it wasn't asked here.

    It does however demonstrate that it's at least possible for the situation described to appear like a deadlock. No one ever gave accurate stats on how long the OP's thread keeps its lock locked. It does say something about the way these locks work in Java… in particular, that they're not first-come first-serve and starvation can clearly be an issue. @dalby said:
    TRWTF is the C is shorter than the Java…
    Why? And why you are using String.format() instead of System.out.format()? Btw, Java has for (;;) too.
    Does any of this matter? The point of the C example was to show another mutex implementation working differently, even given one thread that holds a mutex for 5 seconds at a time. If you think changing the way it writes a string or enters an infinite loop will cause the Java example not to hang, you're free to try it.


  • @Lingerance said:

    A hang.

    Now that we've gotten the irrelevant terminology squabbles out of the way, back to our regularly scheduled WTF: Synchronized everything!

    @Enterprise Architect said:

    No one ever gave accurate stats on how long the OP's thread keeps its lock locked. It does say something about the way these locks work in Java… in particular, that they're not first-come first-serve and starvation can clearly be an issue.

    I let it run overnight by accident (forgot to kill the JUnit process in Eclipse)... it was still frozen when I got back to work the next morning. Is that accurate enough? :)



  • @Lingerance said:

    @Heron said:
    Ok. What term do you use when your unit test never finishes because a thread can't get a lock that another thread is hogging all to itself?
    A hang.

    Lock starvation, actually.



  • Well... it wasn't a deadlock. It may appear to be a deadlock, but if you carefully check out your example you realize it is NOT a deadlock. I'm sorry but you really should provide an example and you have failed. This is not an easy task to do. Concurrent programming seems to be easy to fail ... or is it?



  • If you'd read the thread, you'd realize we've established that it's not a deadlock. It's lock starvation, which looks a lot like, but is not the same as deadlock. Still a WTF, and when it happens, you still find your thread stuck waiting for a lock it'll never acquire.

    TRRWTF is Java, for failing to prevent this. The Java designers should know the typical Java programmer has no clue what any of this means.



  • If you didn't get it... it wasn't a deadlock. Your example wasn't about the deadlock at all. An example which deadlocks with given circumstances would suffice... but I think it is not possible.



  • @dalby said:

    If you didn't get it... it wasn't a deadlock.
    There is no deadlock, in my code or the OP's code. We've established this. Move on.



  • And to mr enterprise architect... you seem to be an example of your kind. You provided an example code (in java AND c) which wasn't related to question at all. And after that you are trying to correct me... please don't. I'm probably better at your job than you are now. If you don't get it, please read again.



  • @dalby said:

    And to mr enterprise architect... you seem to be an example of your kind. You provided an example code (in java AND c) which wasn't related to question at all. And after that you are trying to correct me... please don't. I'm probably better at your job than you are now. If you don't get it, please read again.

    Wow, are you still arguing over silly terminology and ignoring the whole point of the WTF?



  • So the whole point was an utterly stupid commit statement? Wow! You really are the first to post a stupid version control commit statement.



  • The point was a commit comment showing that the developer was obviously excited about having discovered the magic of "synchronized", but the code demonstrated that he didn't understand when to use it and when not to, because it clearly caused locking issues. Why are you being so argumentative about this? It's just a silly little WTF...



  • Easy. its not deadlocks lolz



  • And it is not about terminology... a deadlock is not a starvation. And if you know the basics of computer science you agree.



  • Well.. the code you provided for us to see didn't cause any locking issues or did it? So basically you were bashing him over... nothing? And I'm not being argumentative, I'm just saying your example wasn't causing anything you implied. Sorry for being right.



  • Congratulations! We figured that out.

    Deadlock or not, it's still a bug, and still a WTF. Lock starvation is an issue involving a lock, is it not?





  • liblfds_admin, is that you?



  • @Enterprise Architect said:

    Congratulations! We figured that out.

    Deadlock or not, it's still a bug, and still a WTF. Lock starvation is an issue involving a lock, is it not?

    To quote MST3K: "The bickering EXPLODES across the screen!"



  • @blakeyrat said:

    Filed under: ), now kiss and make up
    But… but…@dalby said:
    Filed under: winning at life, Enterprise architecht
    He misspelled my name! That hurt, ya know! My name and my title are all I have! How dare he disrespect them like that!



  • To have a fucking deadlock you need to have two or more threads waiting for resources that each other has locked.  Like what happens if the two methods below execute and enter their locks concurrently (they'll yield to each other forever):

    Class A {

    object _LockObject1 = new Object();

    object _LockObject2 = new Object();

    void AsynchronousMethod1() {

    lock(_LockObject1) {

    int Len = _LockObject2.toString.Length;

     }

    }

     void AsynchronousMethod2 {

    lock(_LockObject2) {

     int Len = _LockObject1.toString.Length;

    }

     }

    }



  • Wow, why are you so angry over my accidental misuse of terminology, and one we've quite thoroughly corrected already?



  • @hoodaticus said:

    To have a fucking deadlock you need to have two or more threads waiting for resources that each other has locked.

    But on an embedded system with no threadsystem…?

    To have a fucking deadlock, you have to fuck. Please satisfy this precondition.



  • blow me.



  • @dalby said:

    So the whole point was an utterly stupid commit statement? Wow! You really are the first to post a stupid version control commit statement.

    No shit. It's like this entire forum was made for posting stupid things programmers have done. Why can't these people post about goddamn ponies for once?



  •  Man, I love ponies.



  • @hoodaticus said:

    fucking deadlock
    @dhromed said:
    Man, I love ponies.
    Sometimes I am a victim of my own wild imagination.


Log in to reply