- Feature Articles
- CodeSOD
- Error'd
- Forums
-
Other Articles
- Random Article
- Other Series
- Alex's Soapbox
- Announcements
- Best of…
- Best of Email
- Best of the Sidebar
- Bring Your Own Code
- Coded Smorgasbord
- Mandatory Fun Day
- Off Topic
- Representative Line
- News Roundup
- Editor's Soapbox
- Software on the Rocks
- Souvenir Potpourri
- Sponsor Post
- Tales from the Interview
- The Daily WTF: Live
- Virtudyne
Admin
And here I thought MQ was supposed to elegantly queue entries so that you don't have hurricane issues...
Frist BTW
Admin
The thread-pool library I use does that. The first time it's called it spawns however many threads seem sane given the underlying hardware.
Hand it stacks of Runnables or method/parameter/result_placeholder lists and it wakes up the threads does them.
When the threads run out of stuff to do they go to sleep, ready for the next time.
I don't care how many threads there actually are (though I can find out or specify if I really want to), I just make a thread-pool and let it handle it. I don't know if it dynamically resizes the pool if CPUs magically appear during runtime, but given the hardware I target this won't happen so I really don't care.
Admin
Bleurgh.
I've always use commons-pool for this sort of thing: http://commons.apache.org/proper/commons-pool/
Admin
Admin
Admin
This is not a thread-pool. It's a message-queue emptying system that will get into an endless sleep if ever inserting a message fails, leaving the remaining messages forever on hold.
Admin
There's a second WTF in there as well - the InboundMessageHandler will also skip the exit condition of the while loop and happily sleep for another 10ms if getNumberOfDataRows() ever throws an exception querying the number of rows.
Mandatory cartoon: http://geekandpoke.typepad.com/geekandpoke/2009/06/simply-explained-checked-exceptions.html
No Akismet this is not spam.
Admin
erica a linda
Admin
A pleasingly compound WTF.
Admin
this is so much easier to do by just using a thread pool as available in the standard java library
Admin
Aha, the classic "I've read about how to do multithreading so I'm now going to implement my own multithreaded solution " WTF.
Seen this sort of thing many times, especially on Enterprise Java systems, and it invariably causes problems like this, despite the fact that
a) Java app servers handle request / response processing in a multithreaded manner by design in order to support a large volume of traffic, so kicking off your own threads when handling requests is generally stupid and pointless.
b) As pointed out by other posters here, there are several free and robust messaging systems available on the platform, and not using one of them is classic "not invented here" stupidity
c) multithreaded systems are notoriously prone to these type of problems when developers armed with their newly acquired multithreading hammer start to see everything as a nail.
Admin
And this is Java 7 code, so the excuse "but it's old code" doesn't work either.
Admin
A number of things occur to me.
We don't know how InboundMessageHandler is used, so comments about single-threaded operation aren't necessarily accurate. The fact that it extends Thread suggests that they might create several of them that all read from the same queue. This had better be the case because...
Each instance of InboundMessageHandler can handle at most 100 messages per second because of the sleep, except in bizarre cases where the worker thread that gets spawned completes before the first pass through the while() header.
The original problem "merely" needed a cap on the number of threads, nothing more. (I once worked with a Win32 codebase that used a Microsoft framework for handling media packet flows. This framework used one thread per flow, and our application (a VoIP server/gateway) needed several flows per VoIP call. It gave up accepting calls when the total flow load was not far above 1500 flows, and consequently 1500 flow-handler threads, and consequently 1500MB of stack space. Given all the other stuff in memory, those >1500 1MB thread stacks wouldn't all fit... Don't do it this way, boys and girls.)
Admin
wooo... for fun.
Admin
Admin
Admin
One more interesting thought about the system. The article says "in one particular system, the data was coming in way too fast", resulting in so many threads, that the number of threads exceeded the system max.
Now the article does not say whether the data was coming in continuously at a very fast rate or rather in bursts; but if the data is coming in continuously at a very fast rate, than the system will sooner or later face another problem, i.e. that the message queue will grow over time.
If so much data is coming in that immediately assigning a thread for each message will run out of threads, then slowing down the message handling process might prevent the system from running out of threads.
But messages will also get handled later and later with the message queue growing; I don't think that this is a sane situation for the system as well.
Admin
Admin
Have these guys hearing of TPL?
Admin
Oh yeah, the junior can "formally review it" for sure. But she isn't going to be able to provide any criticism beyond the trivial -- it's just a rubber stamp.
Captach: odio -- she'll have to approve the code no matter how odio it is.
Admin
Admin
This is only true when the actions performed are lightweight. If an operation requires multiple seconds or more to finish, the system will feel very unresponsive.
This of course does not necessarily mean you should implement parallel processing yourself, or that you even need a new thread. At least with servlet 3.0 there's some other ways to do this in a cleaner and more efficient manner.
Admin
Admin
(Of course, the top fish wouldn't have been alive unless that atom decayed ...)
Admin
try { Thread.sleep(10); // 10 milliseconds } catch (InterruptedException ie) { // do nothing }
That would have been an immediate red flag for me, no matter what my level of experience is.
Admin
Admin
Most of the time they say:
// TODO: Implement exception handling
Not a red flag for a junior, it will be implemented, right?
Admin
i realy dont like this website
Admin
No, you at the very least log it. If you think otherwise, you have a very, very long way to go in your profession.
Admin
//TODO: don't divide by zero here, the universe doesn't like that
Admin
Except it would always throw InterruptedException since he doesn't call Thread.interrupted() ...
Admin
... and with a slightly higher level of experience, you might realize someday that interrupting a thread is the standard way of asking a thread to clean up after itself and shut down, and the InterruptedException is how that request is received in the thread. So yes, while it may be okay to ignore some exceptions (NumberFormatException comes to mind, if you're sure about your inputs), this isn't really one of those cases.
Admin
Just consider the situation: It will loop until some condition is met. The sleep is there just to avoid busy looping (which is a good thing, apart from the fact that the whole code is a bad idea in the first place). It doesn't matter at all whether or not there is an interruption during a sleep, i.e. if one sleep may last only 5ms instead of 10ms. The loop will finish within 10ms (+ scheduling overhead) of the condition being met either way. And the 10ms are arbitrary anyway, just an upper bound on the waiting time (which may be problematic as has been pointed out, but if 10ms is too long, an occasional shorter sleep is certainly not worse, possibly even a little better).
So again: If there's no problem, don't annoy the user (or admin or whoever reads your log files) with unnecessary noise.
Same situation: If you're looking for a file that can be (validly) in either of two locations, and you don't find it in location A, then find it in B, you don't log the "failure" to find it in A (even if this might be signalled via an exception, depending on how you do it), because finding it in B is alright. If you think otherwise, ask yourself: If you do find it in A, do you then check if it's also in B and log so if not (assuming A and B are equivalent)? And if you do find it in B as well, you should probably log this because one of them is probably redundant. You're now at the point where you log something every time.
Professional doesn't mean blindly following rules, but also applying common sense. Sometimes (not often, though) it is OK to just ignore an exception.
Admin
A proper apex predator would be so proud of his expertise that he would be expecting the juniors to line up outside his office door for the chance of receiving the benefit of his wisdom. He would be desperate to show off his brilliance.
Admin
the real wtf is that all u nerds are gittin ur jimimes in a wads wehn its clearly a bs / story
Admin
I'm just gonna stop you right there and say NO.
Admin
I am amazed how little I like snoofle's writing, even in articles like this. I only look at the byline when I'm disappointed and it's always him... The tone is always so much meaner than the old Alex stories.
Admin
what does "wonded" mean?
Admin
Admin
Admin
And what do you expect your production team to do when they find the offending log entry in the logs? And how will you fix the code when you get a ticket raised by this 'problem'.
The answer is that they can't do anything about it, and there is no way to improve this part of the code (except perhaps a more detailed comment), as fooo mentioned.
But don't let experience and detailed knowledge get in the way of your idealistic blustering. That would take away all the fun for the rest of us.
Admin
Admin
So..... if the thread is being shut down, surely the server is being shut down? Why do you want to log the fact that it happened to be sleeping at the exact moment when the server was being shut down. What do you gain from that?
Or are you imagining some sort of thread manager shutting down the thread because it has taken too long? In that case (with such a short wait time), I would prefer to have the logic inline in this loop, and avoid relying on an exception execution path.
Either way, I'm genuinely interested whether anyone can actually improve on this code. As far as I can tell, the exception is just used as a sort of in-your-face documentation to remind the developer that a sleep operation can be interrupted. I can't remember ever reacting to an interruption in my code, but then again, I mostly do JEE stuff, and spawning threads is a no-no, and I would avoid sleeping at almost all costs.
Admin
There's more than one way to shut down a thread.
Admin
I'm soooooooooooooo glad that I don't work with you.
Admin
You obviously don't want to change your mind, so I won't waste my time. Ignorance is bliss, on multiple levels in this case.
Admin
Obligatory xkcd.
Also Akismet sucks dead goats' fallopian tubes
Admin
Admin
Ugh refactoring threading...
First you try to clean up the logging, because there are all these places they're catching every exception and tedious logging it. So you go back to the thread construction, centralize it, replace the catch(Throwable) with the actual exceptions, and add an uncaught thread death handler since there are invariably a mess of NPEs and such.
Then you start looking at the flow, and naturally they're using the a rat's nest of flags and waits and notifies. You slowly untangle the mess and finally replace it all with a call to join.
And naturally they're not writing data back in a queue, they used something stupid like a Vector, which they've further synchronized on whatever.
And finally it dawns on you that they're not doing anything in the calling thread but waiting for the queue to fill, so the whole thread was completely unnecessary and can be replaced with a simple method call that populates an ArrayList.
Admin
There is no reason to assume that a thread is only shut down when the entire process is terminating.
It only means a certain task is being interrupted. For instance, if I'm doing a file transfer and want the user to be able to click "Cancel," I want to stop the transfer promptly. There's no reason to assume the entire application is shutting down.
Similarly, Tomcat has the ability to load a new version of a web app in place. What it does is it loads the new version and starts handing new requests to the new version. When the old version has no more active requests, it is directed to shut down. (It's pretty slick when it works.)
So a web app in Tomcat needs to release any threads its using in a timely fashion, but it explicitly does not mean that the entire process is terminating.
In most cases, instead of swallowing the exception, you just return or throw some kind of unchecked exception. Or, if you can't handle the interrupt immediately, you use Thread.interrupt() to set the thread's interrupt flag and the next block that can throw an InterruptedException will throw immediately.
Really, you think it makes sense to hammer the database every 10 ms until the number of rows has actually increased?
The commit method in JDBC won't return until the DBMS has actually committed the transaction. You can eliminate all of that and just run the "write message to DBMS" in a regular method call.
If you're interested in better concurrency with a DBMS, you buy an expensive DBMS and tune it. Apps should just send the data in as big batches as they can and only open more connections if it's convenient.