Re: Possible Synchronization Issue

[Harmeet Bedi <ha...@kodemuse.com>]

I think it may be better to have a derived Date Format class that safely
handles mutlithreading issues instead of fixing the code that uses
DateFormat.

As a precedent, this is the approach taken by Log4J and Tomcat.

Harmeet

----- Original Message -----
From: Peter M. Goldstein
To: james-dev@jakarta.apache.org
Sent: Friday, June 28, 2002 12:50 PM
Subject: Possible Synchronization Issue


Hi All,

I'm a developer new to the James project.  In the process of familiarizing
myself with the code base I've found what I believe to be a possible
synchronization issue in the org.apache.james.util.RFC822Date,
org.apache.james.util.RFC822DateFormat, and
org.apache.james.nntpserver.NNTPHandler classes.  Basically the issue
centers on the fact that the format and parse methods of the
java.text.SimpleDateFormat are not thread-safe (they reference a common
internal Calendar instance) and so can only be called in a multi-threaded
context inside an appropriate synchronized block.  Each of these James
classes contains one or more static SimpleDateFormats that are referenced by
a number of instances in an unsynchronized fashion.  Attached are CVS diffs
that resolve this problem - by wrapping the format and parse calls in
appropriate synchronized blocks.

I'm not sure what the process is for a contributor to submit a patch.  I'd
also like to get someone more familiar with the James source code to review
these changes.  So I'm sending this to the james-dev distribution list.  If
this is not the correct procedure, please let me know.  Thanks.

--Peter




--
To unsubscribe, e-mail:   <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>


--
To unsubscribe, e-mail:   <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>

RE: Possible Synchronization Issue

["Peter M. Goldstein" <pe...@yahoo.com>]

Harmeet,

While I don't necessarily disagree with the notion behind the suggested
approach, I can't find the precedents you reference.  I'm especially
confused because, as I understood it, the declaration of a number of
methods in SimpleDateFormat and DateFormat as either final or private
makes the inheritance you describe difficult to pull off.

The Log4J helper classes (package org.apache.log4j.helpers)
ISO8601DateFormat, RelativeTimeDateFormat, and AbsoluteTimeDateFormat
all fail to implement an appropriate parse method.  As we use parse in
the James code base (specifically the NNTPHandler class), these classes
do not meet the requirements for use in James.

Examination of the Tomcat 4.0 source base reveals extensive use of
java.text.SimpleDateFormat with in-situ locking (or lack thereof).
There are a number of classes that appear to do some form or another of
DateFormat functionality, mostly via unsynchronized delegation to
composite SimpleDateFormat objects.  These include use of the
org.apache.catalina.util.DateTool class (essentially a repository for a
set of commonly used SimpleDateFormat objects).  The notable exception
is the org.apache.catalina.util.FastHttpDateFormat class, which has
internal synchronization of a composed SimpleDateFormat object.  This
class doesn't expose the full capabilities of the internalized
SimpleDateFormat object (which is not a problem), nor the signature
(which is not a problem).  But it doesn't extend DateFormat (which is
not a problem).  Of all these, the one that comes closest to what you
describe is FastHttpDateFormat although it doesn't inherit from
java.text.DateFormat.  Please let me know if I've missed something,
because I'm not really familiar with the Tomcat source base.

As I see it, the notion behind your suggestion is correct - we don't
want the clients of the date format classes to have to worry about
synchronization.  Let me make a specific counter suggestion that I think
will address your concerns.

1. Introduce a new interface org.apache.james.util.SimplifiedDateFormat
that lays out the basic methods we want from a date formatting class.
These methods will be a subset of the methods of the DateFormat class.
This interface is attached.

2. Introduce several classes that implement this interface.  These
classes will not inherit from java.text.DateFormat.  These classes
include a SynchronizedDateFormat, which wraps an internal DateFormat in
synchronized blocks.  Other classes include RFC2980DateFormat,
RFC977DateFormat, and a modified version of RFC822DateFormat.  All of
these classes are thread safe.  They are attached.  The first two are
designed to handle the date formats associate with NNTP and described in
RFC 2980 and RFC 977 respectively.  The RFC822DateFormat also includes
the previously present static method toString(Date d) to preserve
backwards compatibility.

3. Change RFC822Date so it delegates to RFC822DateFormat for its format
method.  In truth RFC822Date isn't a great idea.  The only place we use
it in the code is to generate a string representing 'now' in the
appropriate format.  This is just syntactic sugar for
RFC822DateFormat.toString(new Date()) and as such is somewhat
superfluous.  So we deprecate the class, but leave it intact.  This
approach ensures that anyone using this class in mailets and what not is
left with correctly functioning code.  That diff is attached.

4. Replace references to RFC822Date in the assorted classes (there are
three references) with RFC822DateFormat.toString(new Date()) as
described above.  Those diffs (NotifyPostmaster.java.diff,
NotifyPostmaster.java.diff, Redirect.java.diff) are attached.

5. The NNTPHandler should be altered to contain an instance of each of
RFC977DateFormat and RFC2980DateFormat in preference to the current use
of composed SimpleDateFormat objects.  A diff for NNTPHandler is
attached.

6. Change org.apache.james.core.MimeMessageWrapper to use
RFC822DateFormat instead of a synchronized wrapped
javax.mail.internet.MailDateFormat.  This unifies the code.  The
relevant diff is attached.

This is a complete set of changes that fully resolve the thread safety
issues without requiring client classes to implement their own
synchronized blocks.  Your thoughts?

Note that I altered the RFC822DateFormat class (and hence the RFC822Date
class) so that it uses the javax.mail.internet.MailDateFormat class for
formatting.  Comments in this source file indicated a reluctance on the
part of previous authors to do this, but this made little sense to me as
the core classes were employing the MailDateFormat class. 

--Peter