Here are some comments sent by Ervin Varga
ervin.varga@essnet.se.
Here
is the convention used in this document:
COMMENT - This is a bare suggestion or observation regarding some issue from the book not necessarily related to a bug in a code or omission in the text
BUG - This a bug found in a source code published in the book
OVERSIGHT - An errata in the text or a suspicious statement in the book
All remarks conform to the following template:
<category><page number(s)>, where categories are given above.
<introduction>, a short inaugural text clarifying the context for the remark
<theme>, the body of a remark
[<references>], literatures used for a remark, if any
Here is a quote from a book:
"A writing thread releases a synchronization lock and a reading thread subsequently acquires that same synchronization lock."
The above statement is perplexing. The next paragraph in the book, located just beneath of the above statement, clarifies this situation, yet, the quoted declaration is misleading and should be reformulated.
The Java Language Specification contains the following part:
"Any association between locks and variables is purely conventional. Locking any lock conceptually flushes all variables from a thread's working memory, and unlocking any lock forces the writing out to main memory of all variables that the thread has assigned. That a lock may be associated with a particular object or a class is purely a convention. In some applications, it may be appropriate always to lock an object before accessing any of its instance variables, for example; synchronized methods are a convenient way to follow this convention. In other applications, it may suffice to use a single lock to synchronize access to a large collection of objects."
It unmistakably tells that acquiring a same lock is not necessary, simply locking and unlocking unrelated locks is enough, as those are the places where memory synchronization are guaranteed to happen.
James Gosling, Bill Joy, Guy Steele, Gilad Bracha, The JavaTM Language Specification; Second Edition, Addison-Wesley, 2000.
The
book mentions that InheritableThreadLocal
class extends ThreadLocal
without giving some further explanation of the prominent differences
between these two classes.
The
ThreadLocal
class has a relative, InheritableThreadLocal,
which functions in a similar manner, but is suitable for an entirely
different sort of application(s). When a child thread is created, the
child receives initial values for all inheritable thread-local
variables for which the parent has values. If a child thread calls
get
on an InheritableThreadLocal,
it sees the same object as the parent would. To preserve
thread-safety, InheritableThreadLocal
should be used only for immutable objects, because the object is
shared between multiple threads. InheritableThreadLocal
is useful for passing data from a parent thread to a child thread,
such as a User ID, or a Transaction ID, but not for stateful objects
like JDBC Connections.
This
particular property of InheritableThreadLocal
class should be emphasized in the book.
Brian Goetz, Exploiting ThreadLocal to enhance scalability, IBM developerWorks, October 2001.
J2SE 1.4 API documentation
Here is a quote from a book:
"Callback-based handling may also apply when the service itself does not even know which exception should throw upon failure."
In this form the previous statement is a bit controversial, thus, should be reformulated into an unmistakable shape.
Looking from a higher perspective, it is true that sometimes a service does not know the context of its usage, i.e. how it is used in a given situation. This is especially a case for a service provided by a exceedingly reusable component/subsystem. Nevertheless, a service should know about its local execution environment delineated by the component's boundaries. Accordingly, a service should be responsible for throwing an exception containing information only related to a service's own level of abstraction, i.e. knowledge of failure. Higher level parts of the system would then embellish this exception with further context dependent details, probably by nesting the cause exception thrown by a service into a new one. All in all, a service should know which exception to throw; although, it does not need to know the "top" level exception appropriate for upper application layer(s). Conversely, the higher layers will not be able to know what happened inside a service, i.e. on micro-level, if the service does not react to a failure properly (possibly by throwing an appropriate exception). Callbacks used in a manner stated in the quote (see above) would only jeopardize encapsulation and introduce unnecessary couplings.
The concept of nested exceptions is particularly useful in multi-layered designs, where each layer packs the exception caught at lower level into a new adorned one and rethrows it (of course, only in case when it can not handle the exception itself). Moreover, the new 1.4 version of Java 2 directly supports nested exceptions, also known as chained exception facility.
Steve Ball, Exception Handling in Large Systems, Java Report, July 2000.
J2SE 1.4 API documentation
Bill Venners, Exceptions in Java, JavaWorld, July 1998.
Bill Venners, Designing with Exceptions, JavaWorld, July 1998.
Terren Suydam, Use Nested Exceptions in Multitiered Environment, JavaWorld, 1999.
The
remark is associated with the statement in the book telling that a
release
method has to be synchronized as an assurance that a memory
synchronization would occur upon lock release.
The
release
method does not need to be synchronized in this sense, given that the
updated field is volatile,
so the memory is flushed immediately after a value has been written
to a field.
The
remark is associated with the contemplation why method pop
should not be overridden. Here is a quote from a book:
"Among other considerations, the different policies of balking versus waiting are reflected in the different signatures of the methods..."
This is not accurate and could be very confusing for novice Java developers.
The
throws
clause does not contribute to the method's signature. The method
signature is defined in the Java Language Specification as:
"The signature of a method consists of the name of the method and the number and types of formal parameters to the method."
James Gosling, Bill Joy, Guy Steele, Gilad Bracha, The JavaTM Language Specification; Second Edition, Addison-Wesley, 2000.
The
remark is pertaining to a general rule advocated in the book while
describing the Pool
class employing a layered synchronization structure.
There
is a nicely documented architectural pattern called Thread-Safe
Interface [1], which also deals with issues surrounding the
minimization of locking overhead and prevention of 'self-deadlock'.
Of course, the last issue is not so relevant for Java, as it uses
re-entrant monitors. Nevertheless, reacquiring the same lock does
introduce penalties, which could be avoided if the aforementioned
pattern is leveraged. The Pool
class would not actually benefit a lot by applying this pattern, but
other classes could, especially whose inner synchronized methods
(non-public) call each other frequently.
Maybe it would be good to have an example in the book demonstrating this pattern in practice, too.
Douglas Schmidt, Michael Stal, Hans Rohnert, Frank Buschmann, PATTERN-ORIENTED SOFTWARE ARCHITECTURE; Volume 2, Wiley, 2001.
Before delving into a more detail, let us recall couple of important aspects related to the example given in a book (requirements, design & implementation).
On page 241 the following requirements have been stated:
"...consider a service that automatically transfers money from a savings account to a checking account whenever the checking balance falls below some threshold, but only if the savings account is not overdrawn."
On the same page, a bulleted list of items commenced listing observations that should lead to a solution. As we will see later, the implementation has not followed this analysis.
On page 243 the following were written:
"All
this leads to the following very special subclasses of BankAccount,
tuned to work only in their given context."
"The
decision on whether to mark these classes as final
is a close call. However, there is just enough latitude for minor
variations in the methods and protocols not to preclude knowledgeable
subclass authors from, say, modifying the transfer conditions in
shouldTry
or the amount to transfer in transferOut."
This last citation is crucial and we will concentrate on it the next section.
Yes, it is good to provide subclass authors with possibilities to adjust our classes in various ways, therefore, boosting the opportunity for a reuse. But, this also means that we should be tremendously vigilant what should we allow to be changed and how should we enforce proper subclassing. This last topic is interesting enough to be further elaborated, but due to space limitations I will skip it (after all, this is a document describing remarks about a book, and not a book itself). :) The only thing I would like to mention is that a superclass and subclass should have compatible state machines. More on this can be found, for example, in [1], but actually the possibility of swerving from this principle was a principal cause of a trouble. When subclass authors do not take into consideration all those ethics stated in myriad of books dealing with object oriented software design and we do not, as superclass authors, protect ourselves from such 'subclassings' the problems arrive.
Please,
take a look at the base BankAccount
class. As you can see, it is implemented in such a way that it can
never be overdrawn. The ATCheckingAccount
and ATSavingsAccount
classes both extend the BankAccount
class without touching the withdraw
and/or deposit
methods, consequently, they can not be overdrawn either. Moreover, if
you carefully look into the implementation of the ATCheckingAccount
class you will NOT find any point where it protects itself from a
possibility to work with a kind of savings account, which can be
overdrawn (this is a point where implementation does not follow the
result of analysis).
Imagine
a situation when a subclass author extends the ATSavingsAccount
class in a way to let it go into an overdrawn state. The attachment
contains a Java program called AccountTest. (See AccountTest.java.)
There you will find a class ATOverdrawableSavingsAccount
(apparently, this class has an incompatible state machine
compared to the ATSavingsAccount
class). The AccountTest
class demonstrates what happens if someone pass an instance of
ATOverdrawableSavingsAccount
to an ATCheckingAccount
object.
There
is one another drawback with a BankAccount
class. It is semantically nonsense to deposit or withdraw a negative
amount of many to/from an account. Currently, the BankAccount
class allows these kinds of manipulations. It would be more natural
to have clear interface methods deposit(long
amount) and withdraw(long
amount), which would
prohibit such misuses and call an inner one, for example
doDeposit(long amount),
to do the actual work. Moreover, it is always a doubtful design
decision to let an interface method call another interface method of
the same class.
Martin Fowler, Analysis Patterns: Reusable Object Models, Addison-Wesley, 1997.
On page 6 (section §1.1.1.1) in the book the following recommendation can be found:
"Never lock when invoking methods on other objects."
On page 246 you can find the next citation:
"Instead,
here you can apply our default rules from §1.1.1.1
and release unnecessary locks when making calls from Subjects
to Observers,
which serves to implement the desired coupling."
As
wee can see, this is not systematically followed throughout the
implementation of the Observer
class.
The
implementation of the changed
method of the Observer
class does not conform to the rule given on page 6, hence, liveness
problems could emerge for threads executing the aforementioned
method. The changed
method calls display
while still holding the synchronization lock, and it is highly
possible that such a displaying activity will take a considerable
amount of time and call other objects' methods (the implementation
listed in a book is exceedingly simple, yet, it does call another
object's method, namely System.out.println()).
Moreover, since the display
method is declared as protected,
subclasses are free to incorporate more complex displaying
implementations, though.
Threads
executing the changeValue
method of the Subject
class could spend much of their time waiting for Observers
to refresh their displays and this could trigger liveness problems
(one thread is waiting to access the Observer
instance "held" by a second thread just now refreshing a
display).
The
displaying activity should be carefully isolated from the process of
barely informing Observers
about a state change in a Subject.
The book should, at least, contain a note about this particular issue, though.
The
remark is connected to interfaces Transactor
and TransBankAccount
found on pages 253 and 254, respectively.
Explicitly declaring operations in interfaces as public is not only superfluous, but is also considered as an unpleasant programming style.
Here is a quote from a book on page 256:
"But this version suffices to illustrate the general structure of transactional classes and also, implicitly, how much more code would be required to build a more useful version:"
Usefulness
is one thing, but the implementation of the SimpleTransBankAccount
class is also erroneous, because it does not handle properly the
cases when null
values (for a Transaction
argument) are passed to it's transactional methods.
While
an account is not engaged in a transaction the value of the currentTx
field is always set to null
(this is a kind of indication that there is no current transaction).
In this state, all transaction related actions, such as a
deposit/withdraw of money to/from an account, must be prohibited.
Unfortunately, it is quite possible to deposit an amount to the
account just by passing a null
as Transaction
argument and afterward, you can even commit a change in a similar
manner. All in all, this is a classical example what could happen
when special state conditions (such as 'no current transaction') of
objects are not designated and managed properly.
Also,
if you look carefully into the implementation of the aforementioned
class, you will notice that clients can not interrogate the account's
stable balance (represented as a protected
field balance)
while it is engaged in a transaction. A version of a balance
method supporting a special null-transaction type is highly
suggested.
Of
course, it is straightforward to create a new subclass of
SimpleTransBankAccount
class, which would correct the previously mentioned faults. The book
should explicitly advise such an action.
Here is a quote from the book:
"Transaction error. Unrecoverable, catastrophic operation failure can occur if objects fail to commit after they say they can."
The
FailedTransferException
exception is defined to be a checked exception (it extends the
Exception
class) whilst it is more appropriate to be defined as an unchecked
one.
If
you take a look into the implementation of the AccountUser
class you'll notice that a FailureTransferException
exception designates a transaction error (a description for this kind
of error is given in the citation above). Since, it is a severe
error, probably an unrecoverable one, it is not a wise choice to
force clients to always catch and handle this exception (almost
certainly, clients even do not know how to handle it). Checked
exceptions indicate conditions that a reasonable application might
want to catch, but this is not a case with a FailureTransferException
exception.
As
FailureTransferException
exception is related to a rather serious problem (abnormal condition)
it is more appropriate to represent it as a kind of Error.
At least, it should be an unchecked exception. Accordingly, the
AccountUser's
transfer
method should not declare in its throws
clause this exception.
J2SE 1.4 API documentation
The remark is related to an implementation issue pertaining to classes realizing the Sync interface.
If
a waiting thread, which have had invoked the wait
method of the Object
class, gets interrupted by another thread an InterruptedException
will be thrown. The major thing to note here is that the interrupted
status of the current thread is cleared when this exception is
thrown.
All
classes realizing the Sync
interface have to preserve this semantic constraint, that is, the
acquire
and attempt
methods ought to be compatible with a wait
method regarding this matter. This sentence should be attached as an
additional item to the list of observations presented in the book.
The Semaphore
class properly handles this, as all other class mentioned in the book
do, but it would be also good to explicitly publicize this directive.
The
remark is related to an implementation of the
BoundedBufferWithDelegates
class.
In
order to boost the comprehensibility (and perhaps safety, as well) of
the code all three private
fields (array,
putter
and taker)
of the aforementioned class should be declared as final,
though.
The
remark is associated with the meaning of the released
field found in the WaitNode
class (see page 279).
It
should be emphasized in the book that the released
field in the WaitNode
class also guards against missed signals.
Consider
the following scenario. The FIFO semaphore was initialized with some
number of initialPermits
and all those had been "allocated" to different threads.
Any further attempt to acquire a semaphore would automatically
enqueue the request and probably put a thread in a wait state.
Suppose a new thread (in the rest of the text denoted as 'first')
arrives trying to acquire a semaphore. In this condition its request
will be enqueued. Just before hitting the node.doWait
call a thread is switched by another one (denoted as 'second')
releasing a semaphore. Since, there is a request in the queue it will
be processed by setting the released
field of the WaitNode
instance to true
and summoning a notify
call. At this point there is no thread to be notified for this
particular node. If the 'first' thread would have been allowed to
enter wait
(see the implementation of the doWait()
method) after a semaphore release (performed by a 'second' thread)
the signal (a previously mentioned notification) would be missed,
though. In other words, the 'first' thread would not notice that
someone released a semaphore in the meantime.
Here is a quote from a book:
"Provide
a shutdown
method in the worker thread class that causes existing workers to die
and no additional workers to be created."
This method should not be put in the worker thread class.
The
thread pool class has the responsibility of managing workers, so this
is the best place to put the aforementioned method. Actually, in
util.concurrent
package exactly this principle was followed, unfortunately, the
citation from the book is not synchronized with a solution employed
in the formerly mentioned package, though.
The
remark is related to an implemenation of the TimerDaemon
class.
First,
the comment found in the body of the activate
method is not correct. Instead of PlainWorkerThread
PlainWorkerPool
should be referenced (this class is introduced on page 291).
Moreover, the method activate
should be declared as protected,
as it is done in the aforementioned PlainWorkerPool
class.
Second,
the method take
contains an error, it would prematurely run scheduled tasks. The fix
is very simple, the line long waitTime =
now - t.execTime; has to
be changed to long waitTime = t.execTime
- now;.
The
remark is connected with the implementation of the Joiner
class.
The
method tryJoin
has been unnecessarily declared as synchronized.
Since, it is always called from synchronized
methods it is superfluous to attempt to acquire the already held
lock.
Here is a quote from a book:
"This can be a risky strategy, since it will cause indefinite suspension of ready callbacks if one or more of them never completes. This drawback could be addressed by associating time-outs with the waits."
Barely returning as an answer to interruption is not satisfying, too. The book does not contain a warning about this issue.
Let us assume that a client initiated four file reads indexed as 0, 1, 2 and 3. Moreover, presume that threads have completed reading files in the following order 2 0 1 3. If, for example, thread 2 gets interrupted while waiting for its turn then thread 3 will wait indefinitely for its condition to be met.
The
remark is mainly associated with the implementation of the
DiskTaskQueue
class.
The example you've provided (a class controlling read and write access for a disk containing many cylinders but only one read/write head) is an excellent primer for an architectural pattern called Half-Sync/Half-Async, which decouples asynchronous and synchronous service processing in concurrent systems, to make client side API easier for use while at the same time preserving, hopefully increasing, the overall system's performance. It is nicely documented in the book [1].
The
DiskTaskQueue
implementation does not guarantee a full cylinder-based ordering. The
heads of queues (thisSweep
and nextSweep)
are never updated during insertions, even if the newly added
DiskTask's
cylinder number is less than the corresponding head's one.
Here is an example. Suppose a multi-threaded client makes three successive requests to a scheduler (for a sake of brevity assume that all these tasks will be put into the current queue):
scheduler.read(150,
...);
scheduler.write(200,
...);
scheduler.read(100,
...);
The queue will be arranged as 150, 100, 200.