• Inglorious asshole (unregistered)
    task = null; // unnecessary but can help GC

    Is this true?

  • (cs) in reply to Inglorious asshole
    Inglorious asshole:
    task = null; // unnecessary but can help GC Is this true?

    In this case no because the 'loop' is only ever run once, it probably should be something like this:

                while (task != null) {
                runTask(task);
                task = followerTaskOf(task);
            }
  • Warren (unregistered)

    If the ThreadPoolManager doesn't perform all management of the thread pool a) it's not just a thread WTF (which is all too easy), it's a complete OO WTF b) the title for this article is wrong - it's not even a mismanager, more a witness to mismanagement

  • QJo (unregistered)

    "Error occured in afterExecute method."

    TRWTF is the spelling mistake in "occurred".

  • Pista (unregistered)

    I don't see the point trying something if you're not catching anything...

  • RFox (unregistered)

    TRWTF is threads ;-)

  • (cs)

    The real WTF is assuming that IBM (isn't that what's meant with "Big Blue"?) does not offshore.

  • Anonymous (unregistered) in reply to Pista
    Pista:
    I don't see the point trying something if you're not catching anything...
    It makes sense if the finally block actually does cleanup.
  • KnowYourLanguageLibraries (unregistered)

    This hurts ...

    http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/ThreadPoolExecutor.html

  • Extra spicy vindaloo (unregistered) in reply to Inglorious asshole

    Yes it means that the GC can mark it for collection, if it's left assigned, it might be promoted to a later collection.

  • Ron (unregistered)

    I wonder how many more times this will wind up in production, now? Some noob searches for "thread manager," and boom...

  • Anonymous Bigtimer (unregistered) in reply to KnowYourLanguageLibraries
    KnowYourLanguageLibraries:
    This hurts ...

    http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/ThreadPoolExecutor.html

    As someone who wrote a threadpool eerily similar to this (it was literally my first post-college code assignment), I feel the need to stage a partial defense. Given the lack of generics this is probably pre-Java 5, meaning java.util.concurrent didn't exist. And given that it's from a consulting company, I'm guessing they were passing off a new hire as a Java expert (they did it ALL THE TIME at my former employer).

  • (cs)

    C++ programmer should try not to code in Java.

  • JM (unregistered)

    Um, 

    poolCount
    never gets incremented? 
    Worker
    is never instantiated?

  • frz (unregistered)
    The thread pool manager is not thread safe
    While this is correct it also has no public methods, callees are responsible for this.
    Exceptions were not caught, but sent to the console instead of the log4j log file
    This does not happen in the code posted. (I'm assuming that's a slf4j LogFactory)
    The thread pool manager manages creation of threads up to a certain count, but the threads themselves decrement that count upon completion and remove themselves from the array - unsynchronized As threads are removed from the array, the application becomes single threaded over time
    There is no array with threads. (No thread management takes place in th code posted) Also poolSize is only every decremented so addIfUnderMaximumPoolSize will always spawn a new thread and execute it.

    Maybe subclasses show the errors mentioned but the code posted "only" has static fields mutated in non static methods, also not actually managing any threads.

  • (cs) in reply to JM
    JM:
    Um, 
    poolCount
    never gets incremented? 
    Worker
    is never instantiated?
    And: 
    protected List workers = new ArrayList();
    Threads are never added to this workers-list!
  • (cs) in reply to frz
    frz:
    Exceptions were not caught, but sent to the console instead of the log4j log file
    This does not happen in the code posted. (I'm assuming that's a slf4j LogFactory)
    Actually it does happen in the runTask-method: 
        private void runTask(Runnable task) {
...
            try {
                task.run();
                ran = true;
                ++completedTaskCount;
            } catch (RuntimeException ex) {
                logger.error("Error occured in afterExecute method.", ex);
                throw ex;
            }
...
    }
    Bonus WTF: The log-message claims an Error (not an Exception, but an Error, which is a different thing) occured in method "afterExecute", not in "runTask"!
  • Doctor_of_Ineptitude (unregistered) in reply to Nagesh
    Nagesh:
    C++ programmer should try not to code in Java.

    How did you divine upon such enlightenment?

    It's one thing to say one should not carry their C++ habits while coding in Java, its entirely different if you accuse C++ programmers of being crap Java programmers. And for the defense, for the amount of hate C++ is getting these years, it still is a very good and very versatile programming language. And personally, sometimes, an array of function pointers is way better than a lengthy switch case.

  • Aralmo (unregistered) in reply to no laughing matter

    I would change that for the exception is swallowing the inner exception by using throw ex; instead of just throw;

  • eros (unregistered) in reply to Aralmo
    Aralmo:
    I would change that for the exception is swallowing the inner exception by using throw ex; instead of just throw;
    Please point out where in the specification you see any indication that "just throw;" is valid Java.
  • Anonymous Cowherd (unregistered) in reply to JM
    JM:
    Worker
    is never instantiated?

    This is probably for the best:

    • "Main run loop" is not a loop
    • If it were a loop, it would probably grab all of the tasks out of the list and run them all at once, because instead of extending Thread and executing the task's run() directly, it implements Runnable and spawns yet another thread to do the actual work.

    Captcha: secundum. Not quite frist.

  • (cs) in reply to Doctor_of_Ineptitude
    Doctor_of_Ineptitude:
    And personally, sometimes, an array of function pointers is way better than a lengthy switch case.
    So that's why there are lambda expressions in Java 8. Not that they're particularly easy to understand, or maybe I'm just too old.
  • (cs) in reply to Aralmo
    Aralmo:
    I would change that for the exception is swallowing the inner exception by using throw ex; instead of just throw;
    Are you buttuming this is C#?

    It is not. It is Java where you always must throw an exception.

    In fact this is classic WTFecption-Handling: The excpetion is not handled at all, but the log file is polluted with noise. The actual exception-handler will almost certainly also log the exception, so it is logged twice and confuses the reader.

    What you should do is either not handle the exception at this place or create a different (layer-specific) exception which contains the original exception as cause, so that teh actual exception handler has the required information to handle the exception and logs it only once!

  • Hmmmm (unregistered) in reply to Pista
    Pista:
    I don't see the point trying something if you're not catching anything...

    Sex & syphilis...

  • (cs)

    Clearly, whoever wrote this is one of those people who had heard that "synchronized" is evil, and so are locks.

    Either that or else he had a strong belief in his rabbit's foot.

  • Dan (unregistered) in reply to Inglorious asshole
    Inglorious asshole:
    task = null; // unnecessary but can help GC

    Is this true?

    In very early versions of Java (e.g. 1.1) it was true. In later versions it actually hurt. Not sure the current state, but in any case this is a great example the type of optimization you shouldn't try to do.

  • (cs) in reply to no laughing matter
    no laughing matter:
    Aralmo:
    I would change that for the exception is swallowing the inner exception by using throw ex; instead of just throw;
    Are you buttuming this is C#?

    It is not. It is Java where you always must throw an exception.

    In fact this is classic WTFecption-Handling: The excpetion is not handled at all, but the log file is polluted with noise. The actual exception-handler will almost certainly also log the exception, so it is logged twice and confuses the reader.

    What you should do is either not handle the exception at this place or create a different (layer-specific) exception which contains the original exception as cause, so that teh actual exception handler has the required information to handle the exception and logs it only once!

    That sounds exactly like how things are done at my company.

  • (cs) 
    // fall through

    Should be:

    // fail through
            } finally {
            // intentionally left blank
        }

    If it's left blank, it doesn't need to be there...

  • (cs) in reply to no laughing matter
    no laughing matter:
    Aralmo:
    I would change that for the exception is swallowing the inner exception by using throw ex; instead of just throw;
    Are you buttuming this is C#?

    It is not. It is Java where you always must throw an exception.

    In fact this is classic WTFecption-Handling: The excpetion is not handled at all, but the log file is polluted with noise. The actual exception-handler will almost certainly also log the exception, so it is logged twice and confuses the reader.

    What you should do is either not handle the exception at this place or create a different (layer-specific) exception which contains the original exception as cause, so that teh actual exception handler has the required information to handle the exception and logs it only once!

    +1+1+1+1+1+1+1+1+1+1+1+1+1

  • (cs) in reply to chubertdev
    chubertdev:
    no laughing matter:
    Aralmo:
    I would change that for the exception is swallowing the inner exception by using throw ex; instead of just throw;
    Are you buttuming this is C#?

    It is not. It is Java where you always must throw an exception.

    In fact this is classic WTFecption-Handling: The excpetion is not handled at all, but the log file is polluted with noise. The actual exception-handler will almost certainly also log the exception, so it is logged twice and confuses the reader.

    What you should do is either not handle the exception at this place or create a different (layer-specific) exception which contains the original exception as cause, so that teh actual exception handler has the required information to handle the exception and logs it only once!

    That sounds exactly like how things are done at my company.

    company of misfit people?

  • garaden (unregistered) in reply to no laughing matter
    no laughing matter:
    Are you buttuming this is C#?

    Word filter, or awesome wordplay? Either way it burned my cheese pretty good.

  • Mason Wheeler (unregistered) in reply to Doctor_of_Ineptitude
    Doctor_of_Ineptitude:
    Nagesh:
    C++ programmer should try not to code in Java.

    How did you divine upon such enlightenment?

    It's one thing to say one should not carry their C++ habits while coding in Java, its entirely different if you accuse C++ programmers of being crap Java programmers.

    What if you just accuse them of being crap programmers in general?

    And for the defense, for the amount of hate C++ is getting these years, it still is a very good and very versatile programming language. And personally, sometimes, an array of function pointers is way better than a lengthy switch case.

    And if function pointers were exclusive to the C++ language, you might have a valid point there, but they aren't. C++ is one giant, steaming mass of WTF from beginning to end. It may not be the worst programming language ever created, but it is without a doubt the worst ever to be taken seriously.

  • (cs) in reply to garaden
    garaden:
    no laughing matter:
    Are you buttuming this is C#?

    Word filter, or awesome wordplay? Either way it burned my cheese pretty good.

    It's a clbuttic mistake.

  • (cs) in reply to garaden
    garaden:
    no laughing matter:
    Are you buttuming this is C#?

    Word filter, or awesome wordplay? Either way it burned my cheese pretty good.

    More like a meme on this page; not my invention.

  • Me (unregistered) in reply to Coyne

    Synchronized is evil, and so are locks if you don't know what you are doing. Hell, multi-threaded code in general is evil if you don't know what you are doing. This looks suspiciously similar to something I once worked with at some bluish company once. The situation there was a whole bunch of db code that constantly hit race conditions, deadlocks, and other concurrency issues. The team working on it absolutely refused to touch any Java 5 stuff including their concurrency support because they wanted to keep it compatible with Java 1.4 (the rest of the project was on Java 5, but they were convinced they might get other projects throughout the company to use it as well, and those projects might be on Java 1.4). So the result was an in house developed thread pool was made. And while the original author might have known what he was doing, things quickly devolved as more and more people had to maintain it.

  • Butthats 4Eva (unregistered) in reply to no laughing matter
    no laughing matter:
    Aralmo:
    I would change that for the exception is swallowing the inner exception by using throw ex; instead of just throw;
    Are you buttuming this is C#?

    What did you expect in a forum filled with butthats?

  • quat (unregistered) in reply to Butthats 4Eva
    Butthats 4Eva:
    no laughing matter:
    Aralmo:
    I would change that for the exception is swallowing the inner exception by using throw ex; instead of just throw;
    Are you buttuming this is C#?

    What did you expect in a forum filled with butthats?

    It's all fun and games until someone gets buttbuttinated.

  • Masaaki (unregistered) in reply to quat

    Buttbuttin's Creed.

  • Masaaki (unregistered) in reply to Masaaki
    Masaaki:
    Buttbuttin's Creed.

    Dammit, forgot to hit the Quote button.

    This was in reply to

    "It's all fun and games until someone gets buttbuttinated"

  • groovy gstring nsfw (unregistered) in reply to quat
    quat:
    Butthats 4Eva:
    no laughing matter:
    Aralmo:
    I would change that for the exception is swallowing the inner exception by using throw ex; instead of just throw;
    Are you buttuming this is C#?

    What did you expect in a forum filled with butthats?

    It's all fun and games until someone gets buttbuttinated.

    win

  • redwizard (unregistered) in reply to Ron
    Ron:
    I wonder how many more times this will wind up in production, now? Some noob searches for "thread manager," and boom...

    TRWTF is when said noob, upon questioning as to how they came up with this brillant "thread manager", helpfully shows you their source: a Google search response link pointing to...thedailywtf.com.

  • (cs) in reply to groovy gstring nsfw
    groovy gstring nsfw:
    quat:
    Butthats 4Eva:
    no laughing matter:
    Aralmo:
    I would change that for the exception is swallowing the inner exception by using throw ex; instead of just throw;
    Are you buttuming this is C#?

    What did you expect in a forum filled with butthats?

    It's all fun and games until someone gets buttbuttinated.

    win

    you will be buttimilated.

  • (cs)

    TRWTF is protected fields (protected STATIC fields?!) and no generics.

  • Konnledt Scheisswerk (unregistered)

    Use threads? Now you have nThreads problems.

  • QJo (unregistered) in reply to Masaaki
    Masaaki:
    Masaaki:
    Buttbuttin's Creed.

    Dammit, forgot to hit the Quote button.

    This was in reply to

    "It's all fun and games until someone gets buttbuttinated"

    This is turning into a bit of a compebreastion.

  • (cs) in reply to Anonymous Cowherd
    /** * Thread this worker is running in. Acts as a final field, * but cannot be set until thread is created. */
    It isn't set at all.
  • (cs)

    I like how it slaughters each thread with a NullPointerException (the worker's thread field is never assigned) that bubbles out of a finally block, possibly even nested finallys. That's just magic.

    Best code WTF in ages. There's just so much to “love” in there.

  • C10B (unregistered) in reply to Pista

    It's almost replicating the old VB6 "On Error Resume Next" - a great facility to bypass any knotty problems with ease :-)

  • Over-anal-yzed (unregistered) in reply to QJo
    QJo:
    Masaaki:
    Masaaki:
    Buttbuttin's Creed.

    Dammit, forgot to hit the Quote button.

    This was in reply to

    "It's all fun and games until someone gets buttbuttinated"

    This is turning into a bit of a compebreastion.

    Oh yeah? Real programmers use butterflies!

  • (cs) in reply to Over-anal-yzed
    Over-anal-yzed:
    Oh yeah? Real programmers use butterflies!

    I never use any butterfly in programming.

Leave a comment on “The Thread Mismanager”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article