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?



  • @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 😛 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 😛

    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…


Log in to reply
 

Looks like your connection to What the Daily WTF? was lost, please wait while we try to reconnect.