Web Development
by kevlin
The approach described in the article is misnamed and attempts to tackle the wrong problem. The misconception occurs at a number of levels, with the unfortunate result that if you want robust, efficient and thread-safe code, avoiding the recommendations would be a better approach than adopting them.
The article is well meant, but unfortunately it not only addresses the problem inadequately, it addresses the wrong problem. It attempts to address symptoms rather than causes, with the effect ofadding workaround code. Invariably, the cleanest solutions will be a much shorter and clearer one because they will be focused on the code that should (and should not) be written to create a working system, rather than a call-response strategy basedon finding problems and working around them.
But first, the title of the article and technique: the technique is not at all about copying an iterator. The proposed technique copies the target collection. Because copying an iterator would make precisely no difference to anything in Java, I wondered initially at the title and the wording of the article.
Kevlin
by kevlin
Deconstructing a little, what the article is saying is that if you want to write thread-safe code, instead of thinking about the needs of each task and the relationships between threaded tasks, you should create an iterator to copy the entire contents of the target collection and then iterate over the copied collection, which will be created as a linked list.
This makes sense only if efficiency is not really a concern (or to be precise, if inefficiency is acceptable): the use of the toArray method on collections offers a much briefer and more efficient (wrt both time and space) mechanism than running through the elements manually. The presented solution therefore works at the wrong level: it is focused on the iterator, but the path to an appropriate solution is wrt the collection.
There are cases where this pattern of copying the contents and iterating over the resulting copy make sense, but such solutions are point solutions rather than general solutions. In such solutions, it is important that the process of taking the copy can be guaranteed to be thread-safe.
And this is where another problem occurs. The presented code chooses to lock the collection in order to make a copy of it. However, other than implementing a List interface the concrete collection type in question is unknown. There is a subtle bug waiting here: the synchronize may have no effect, which means that the code is potentially unsafe.
A simple recommendation that many programmers overlook is: never synchronize through interfaces. If you don't know what type it is, how do you know that it provides meaningful synchronisation? The object you have just synchronized could be a wrapper, so you will synch the wrapper but not the real collection. The object you have just synchronized may not use its monitor for synchronisation, using instead one or more private objects.
Kevlin
by kevlin
Stepping back a moment, it becomes apparent what the deep problem is: don't share between threads objects that weren't designed to be shared between threads. In this case, either share a wrapped collection (the Collections API offers this feature) or, more sensibly, don't share collections between threads. Either share objects that are immutable or share objects that are intended to act as keepers between threads, which means that rather than fully generic interfaces, they will have interfaces designed with both concurrency and the concepts of the application in mind.
OO design encourages programmers to think about the nature of the conversation between objects, which means that two threads in conversation will probably have something more meaningful to say than what is on offer from either the Iterator or Collection interfaces.
Kevlin
Thanks for the analysis Kevlin. A few replies, though in general I can't fault many of your points.
A couple of things from my point of view, or at least my belief/stance.
Fixing symptoms is an important set of tricks to have up ones sleeve. In this case it can turn a common error into a rare error. This is 'bad' programming in theory, but quite good sometimes in practice, especially for cost of time spent limiting the bug to rare occasions. It will however come back and bite later.
Performance of the iterator. I'm against returning arrays from methods, instead I prefer to return the much cleaner Iterator interface. I could improve the performance though by using a toArray as you say and then wrapping an ArrayIterator [from some external library such as Apache's Collections] around it.
With hindsight, trying to find a simple tip on threading is never going to happen, and I failed to hit the mark on this one. My apologies.
Hen
This discussion is closed to new replies.
Sort by Trending