You are viewing a plain text version of this content. The canonical link for it is here.
Posted to server-dev@james.apache.org by "Peter M. Goldstein" <pe...@yahoo.com> on 2002/10/19 23:00:43 UTC
Latest diffs, source code
All,
Attached is a set of source files and diffs that should allow you to
reproduce the code base that Noel and I have been using for our latest
testing.
The test ran for over seven hours, with a rate that started close to
~3000 messages/minute and stabilized at ~2500 messages/minute. We don't
have a good explanation for this change, but the system was under a
great deal of stress (logging, TCP/IP, etc.) that may have accounted for
the eventual degradation.
Total number of threads was stable at 112 for the first hour or two, but
grew to 162 threads after that and stabilized. Analysis of the logs
(~750 MB of them) confirmed that there was no thread leak (all worker
threads were reused as expected over the course of the test run). My
suspicion is that the growth in the thread pool size was a result of a
thread scheduler statistical anomaly (disposed but not exited connection
handler/watchdog threads) and is not an actual cause for concern.
Unfortunately this is difficult to confirm or refute, but it matches the
observed behavior.
Java heap size grew slowly over the course of the test, from about 4.5
MB to about 5MB. We don't see this as a real cause for concern,
considering the number of open resources, etc. that the product was
managing.
The code executing threads from the thread pool is now appropriately
wrapped in try/catch/finally blocks that ensure that a temporarily empty
thread pool does not constitute a fatal error. If a thread is
unavailable, the connection will simply be refused. Obviously, if the
thread pool is too small (i.e. no threads available at any time to
service connections), then this will result in an unresponsive server.
But the current configuration has proved to be more than adequate for
our test.
Noel ended the test when he saw a notable performance degradation (drop
to <1000 messages/minute). He later discovered that some other work was
being done on the same server at that time that very well may have been
responsible for the drop in performance. So we don't actually know what
caused this issue and it may not actually be an issue at all. If Noel
and I can find the time we will attempt to run another test to get info
on this one way or another.
So here's the summary:
i) James' SMTP handler can process 2500-3000 messages/minute
consistently on a PII 400 MHz Celeron running Linux
ii) In the source of seven hours, over 1,000,000 messages were pushed
through the James SMTP service.
iii) There was no sign of the catastrophic memory leaks that plagued
earlier versions of James.
iv) There may be some outstanding "glitches" but they are orders of
magnitude less severe than the previous issues.
--Peter
Re: Latest diffs, source code
Posted by Serge Sozonoff <se...@globalbeach.com>.
Hi Peter,
Yup, that almost solves it, but you forgot:
theHandler.setMailServer(mailServer);
Thanks, Serge
----- Original Message -----
From: "Peter M. Goldstein" <pe...@yahoo.com>
To: "'James Developers List'" <ja...@jakarta.apache.org>
Sent: Tuesday, October 22, 2002 9:17 PM
Subject: RE: Latest diffs, source code
>
> Yup.
>
> I can even tell you what that specific problem is. The RemoteManager is
> the rev I posted is not setting the UsersRepository. See the
> newHandler() method in RemoteManager. Modify it so that it calls
> theHandler.setUsersStore(usersStore) and
> theHandler.setUsersRepository(users) immediately before the
> theHandler.setWatchdog call.
>
> That set of changes - isolating the handler creation - was one of the
> last I made before making the post, so I'm not surprised I introduced
> that error. Sorry for the bother.
>
> --Peter
>
> > -----Original Message-----
> > From: Serge Sozonoff [mailto:serge@globalbeach.com]
> > Sent: Tuesday, October 22, 2002 12:07 PM
> > To: James Developers List; farsight@alum.mit.edu
> > Subject: Re: Latest diffs, source code
> >
> > Hi Peter,
> >
> > I am running the latest cvs version of James with your patches and
> > additional files.
> > Java SDK 1.3.1_06
> > Solaris 8 on a SUN SPARC
> >
> > I came across this error when trying to add a user using the remote
> > manager
> > to a fresh install of James (no user database yet and file based)
> > Basically it pukes with an NPE when I try and add a user :-(
> > Anything changed lately that could be causing this?
> >
> > Thanks, Serge
> >
> >
> >
> > JAMES Remote Administration Tool 2.1a1-cvs
> > Please enter your login and password
> > Login id:
> > root
> > Password:
> > ******
> > Welcome root. HELP for a list of commands
> > adduser test test
> > Exception: null
> > Connection closed by foreign host.
> >
> >
> > ==== Log Extract ====
> >
> > 22/10/02 19:30:18 INFO remotemanager: Login for root successful
> > 22/10/02 19:30:27 ERROR remotemanager: Encountered exception in
> handling
> > the
> > remote manager connection.
> > java.lang.NullPointerException
> > at
> >
> org.apache.james.remotemanager.RemoteManagerHandler.doADDUSER(RemoteMana
> ge
> > rH
> > andler.java:447)
> > at
> >
> org.apache.james.remotemanager.RemoteManagerHandler.parseCommand(RemoteM
> an
> > ag
> > erHandler.java:390)
> > at
> >
> org.apache.james.remotemanager.RemoteManagerHandler.handleConnection(Rem
> ot
> > eM
> > anagerHandler.java:321)
> > at
> >
> org.apache.james.util.connection.ServerConnection$ClientConnectionRunner
> .r
> > un
> > (ServerConnection.java:364)
> > at
> >
> org.apache.avalon.excalibur.thread.impl.ExecutableRunnable.execute(Execu
> ta
> > bl
> > eRunnable.java:47)
> > at
> >
> org.apache.avalon.excalibur.thread.impl.WorkerThread.run(WorkerThread.ja
> va
> > :8
> > 0)
> > 22/10/02 19:30:27 INFO remotemanager: Logout for root.
> >
> >
> >
> >
> >
> > --
> > To unsubscribe, e-mail: <mailto:james-dev-
> > unsubscribe@jakarta.apache.org>
> > For additional commands, e-mail: <mailto:james-dev-
> > help@jakarta.apache.org>
>
>
>
> --
> 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: Latest diffs, source code
Posted by "Peter M. Goldstein" <pe...@yahoo.com>.
Yup.
I can even tell you what that specific problem is. The RemoteManager is
the rev I posted is not setting the UsersRepository. See the
newHandler() method in RemoteManager. Modify it so that it calls
theHandler.setUsersStore(usersStore) and
theHandler.setUsersRepository(users) immediately before the
theHandler.setWatchdog call.
That set of changes - isolating the handler creation - was one of the
last I made before making the post, so I'm not surprised I introduced
that error. Sorry for the bother.
--Peter
> -----Original Message-----
> From: Serge Sozonoff [mailto:serge@globalbeach.com]
> Sent: Tuesday, October 22, 2002 12:07 PM
> To: James Developers List; farsight@alum.mit.edu
> Subject: Re: Latest diffs, source code
>
> Hi Peter,
>
> I am running the latest cvs version of James with your patches and
> additional files.
> Java SDK 1.3.1_06
> Solaris 8 on a SUN SPARC
>
> I came across this error when trying to add a user using the remote
> manager
> to a fresh install of James (no user database yet and file based)
> Basically it pukes with an NPE when I try and add a user :-(
> Anything changed lately that could be causing this?
>
> Thanks, Serge
>
>
>
> JAMES Remote Administration Tool 2.1a1-cvs
> Please enter your login and password
> Login id:
> root
> Password:
> ******
> Welcome root. HELP for a list of commands
> adduser test test
> Exception: null
> Connection closed by foreign host.
>
>
> ==== Log Extract ====
>
> 22/10/02 19:30:18 INFO remotemanager: Login for root successful
> 22/10/02 19:30:27 ERROR remotemanager: Encountered exception in
handling
> the
> remote manager connection.
> java.lang.NullPointerException
> at
>
org.apache.james.remotemanager.RemoteManagerHandler.doADDUSER(RemoteMana
ge
> rH
> andler.java:447)
> at
>
org.apache.james.remotemanager.RemoteManagerHandler.parseCommand(RemoteM
an
> ag
> erHandler.java:390)
> at
>
org.apache.james.remotemanager.RemoteManagerHandler.handleConnection(Rem
ot
> eM
> anagerHandler.java:321)
> at
>
org.apache.james.util.connection.ServerConnection$ClientConnectionRunner
.r
> un
> (ServerConnection.java:364)
> at
>
org.apache.avalon.excalibur.thread.impl.ExecutableRunnable.execute(Execu
ta
> bl
> eRunnable.java:47)
> at
>
org.apache.avalon.excalibur.thread.impl.WorkerThread.run(WorkerThread.ja
va
> :8
> 0)
> 22/10/02 19:30:27 INFO remotemanager: Logout for root.
>
>
>
>
>
> --
> To unsubscribe, e-mail: <mailto:james-dev-
> unsubscribe@jakarta.apache.org>
> For additional commands, e-mail: <mailto:james-dev-
> help@jakarta.apache.org>
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: Latest diffs, source code
Posted by Serge Sozonoff <se...@globalbeach.com>.
Hi Peter,
I am running the latest cvs version of James with your patches and
additional files.
Java SDK 1.3.1_06
Solaris 8 on a SUN SPARC
I came across this error when trying to add a user using the remote manager
to a fresh install of James (no user database yet and file based)
Basically it pukes with an NPE when I try and add a user :-(
Anything changed lately that could be causing this?
Thanks, Serge
JAMES Remote Administration Tool 2.1a1-cvs
Please enter your login and password
Login id:
root
Password:
******
Welcome root. HELP for a list of commands
adduser test test
Exception: null
Connection closed by foreign host.
==== Log Extract ====
22/10/02 19:30:18 INFO remotemanager: Login for root successful
22/10/02 19:30:27 ERROR remotemanager: Encountered exception in handling the
remote manager connection.
java.lang.NullPointerException
at
org.apache.james.remotemanager.RemoteManagerHandler.doADDUSER(RemoteManagerH
andler.java:447)
at
org.apache.james.remotemanager.RemoteManagerHandler.parseCommand(RemoteManag
erHandler.java:390)
at
org.apache.james.remotemanager.RemoteManagerHandler.handleConnection(RemoteM
anagerHandler.java:321)
at
org.apache.james.util.connection.ServerConnection$ClientConnectionRunner.run
(ServerConnection.java:364)
at
org.apache.avalon.excalibur.thread.impl.ExecutableRunnable.execute(Executabl
eRunnable.java:47)
at
org.apache.avalon.excalibur.thread.impl.WorkerThread.run(WorkerThread.java:8
0)
22/10/02 19:30:27 INFO remotemanager: Logout for root.
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: Latest diffs, source code
Posted by "Noel J. Bergman" <no...@devtech.com>.
> By looking at the code it looked like James is using only execute
signature
> on ThreadPool and this matches very well with PooledExecutor#execute
method
> in util.concurrent.
Yes, but they actually work very differently. It appears to me that we'd
have run into other issues if we were using util.concurrent. For example,
since PooledExecutor's workers don't clear the interrupted state, if a
Runnable checked that state at the top of an internal loop, it would
immediately exit. Second, PooledExecutor takes an interesting approach to
thread pooling, and does not actually do any pooling unless there is a
backlog of tasks. This would have had the effect of masking the interrupted
state issue until we ran at a heavy load factor. I'd have to write some
tests, but that appears to be the case from reading the code.
> Seems harmless unless some code is checking for Thread#isInterrupted.
> That does not seem likely as Thread#interruped is in finally clause.
The purpose of the finally clause is so that when James is done with the
thread, we give it back to the thread pool in a pristine state. It is the
last thing that the handler does, so presumably any of our code that cared
about the interrupted state is already finished with it.
> Why should Thread#interrupt ever be called other than at system
> shutdown time?
For the usual reasons of calling Thread#interrupt. At one point, Peter was
calling it in a version of his Watchdog trigger. When we switch to
java.nio, you would call it rather than the somewhat ruder mechanism of just
closing the socket.
> To me desired state for reuse should be default state. State should be
reset
> by pool after use. However there does not seem to be happening either in
my
> snapshot of Avalon or util.concurrent. It almost seems that plugin code
into
> a thread pool is not expected or should not alter thread state.
This is why we added the finally clause to reset the state. And why I
commented that the JSR 166 group should document normative behavior.
--- Noel
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: Latest diffs, source code
Posted by Harmeet Bedi <ha...@kodemuse.com>.
----- Original Message -----
From: "Noel J. Bergman" <no...@devtech.com>
Subject: RE: Latest diffs, source code
> We'd have to replace the DefaultThreadManager block. My concern is that
> without auditing the code, I don't know, a priori, that we wouldn't
> introduce new problems by mixing and matching between util.concurrent and
> Excalibur. Avalon is already planning a JSR 166 pickup, and James is
> currrently running OK with their code.
true...
By looking at the code it looked like James is using only execute signature
on ThreadPool and this matches very well with PooledExecutor#execute method
in util.concurrent. Was a bit allured by the close match.
>
> > > finally {
> > > Thread.currentThread().interrupted();
> > > }
>
> > Would this be needed post threading fixes in Avalon ?
>
> Probably not, but why not clean up after ourselves?
Seems harmless unless some code is checking for Thread#isInterrupted. That
does not seem likely as Thread#interruped is in finally clause. Is that
right ?
One question: Why should Thread#interrupt ever be called other than at
system shutdown time ? Is there a piece of code that calls Thread#interrupt
other than at system shutdown ? Looked some but could not find a specific
place wondering if you knew...
> If we did replace the
> ThreadPool implementation, what requirements might it have? This is a
good
> issue tidbit to raise with the JSR 166 folks. There ought to be a defined
> semantic for working with thread pools. Either Runnables have to catch
> exceptions and clean up, or they don't. For example, Commons ThreadPool
(in
> their sandbox) says that "inside the run() method of your Runnable object
> you must handle Exceptions properly." The docs don't mention other thread
> state issues.
To me desired state for reuse should be default state. State should be reset
by pool after use. However there does not seem to be happening either in my
snapshot of Avalon or util.concurrent. It almost seems that plugin code into
a thread pool is not expected or should not alter thread state.
Harmeet
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: Latest diffs, source code
Posted by "Noel J. Bergman" <no...@devtech.com>.
> I was not suggesting we decouple. I was suggesting that we implement the
> ThreadPool Manager abstraction using util.concurrent at least the part of
> ThreadPool abstraction that James uses.
We'd have to replace the DefaultThreadManager block. My concern is that
without auditing the code, I don't know, a priori, that we wouldn't
introduce new problems by mixing and matching between util.concurrent and
Excalibur. Avalon is already planning a JSR 166 pickup, and James is
currrently running OK with their code.
> > finally {
> > Thread.currentThread().interrupted();
> > }
> Would this be needed post threading fixes in Avalon ?
Probably not, but why not clean up after ourselves? If we did replace the
ThreadPool implementation, what requirements might it have? This is a good
issue tidbit to raise with the JSR 166 folks. There ought to be a defined
semantic for working with thread pools. Either Runnables have to catch
exceptions and clean up, or they don't. For example, Commons ThreadPool (in
their sandbox) says that "inside the run() method of your Runnable object
you must handle Exceptions properly." The docs don't mention other thread
state issues.
--- Noel
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: Latest diffs, source code
Posted by Harmeet Bedi <ha...@kodemuse.com>.
----- Original Message -----
From: "Noel J. Bergman" <no...@devtech.com>
> AFAIK, replacing the abstraction could require decoupling the handler
> classes entirely from Avalon Frameworks. ....
> why would we want to decouple from Avalon now?
I was not suggesting we decouple. I was suggesting that we implement the
ThreadPool Manager abstraction using util.concurrent at least the part of
ThreadPool abstraction that James uses.
I guess if thread library is stable in Avalon there is no need to make that
effort. I have been out of Avalon lists for a long time, but it was my
impression that there have been on and off threading issues. It would have
been better to have relied on a standard threading library because it is
hard(at least to me) to get an optimal combination of correctness and
performance in mutilthreaded code.
The general philosophy I was trying to suggest is this - get best of breed
and plugin, while maintaining the same abstractions. Trim or suggest to
Avalon folks a narrowing of abstraction from James experience.
[hb]
> > can you point out where the thread pool finally changes have been done.
> > Can't see from the diffs and how this is a problem with current system.
[noel]
>
> finally {
> Thread.currentThread().interrupted();
> }
>
> Technically, Thread.interrupted() would suffice; currentThread() is
> redundant.
Wow, relying on Thread#interrupted to clear interrupted state. It must have
been a huge effort to find this workaround. Would this be needed post
threading fixes in Avalon ?
Harmeet
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: Latest diffs, source code
Posted by "Noel J. Bergman" <no...@devtech.com>.
> > What "scratchpad thread pool"? James uses Thread pool management
provided
> > by Excalibur. Are you proposing that we replace their thread management
> > code with new classes derived from Doug Lea's library?
> I think we should [replace Excalibur's thread management
> with code derived from util.concurrent]. Replacing thread
> pool abstraction should be straight forward.
I don't disagree that util.concurrent is good code, and there are some
things that I prefer in it to the analogous Avalon code from Excalibur, but
we only found one bug in their code, which (a) we've worked around and, (b)
they've fixed in their CVS.
AFAIK, replacing the abstraction could require decoupling the handler
classes entirely from Avalon Frameworks. Since the Avalon folks are working
directly with Doug Lea, and tracking JSR 166, with the intent of using the
result of JSR 166 in some future version of the Avalon code, why would we
want to decouple from Avalon now?
> Thread pool code was part of scratchpad where the Avalon folks had useful
> but not fully qualified code. It looks like it has been moved to not sure
> where.
It is part of Avalon-Excalibur.
> > The ThreadPool and MimeMessageInputStream changes *are*
> > independent of the Watchdog / Scheduler issue.
> Had to wade through a alot of code, so could not easily
> separate. Thanks for pointing out.
You're welcome.
--- Noel
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: Latest diffs, source code
Posted by Harmeet Bedi <ha...@kodemuse.com>.
----- Original Message -----
From: "Noel J. Bergman" <no...@devtech.com>
> > A. Problem with MimeMessageInputStreamSource.java
> > Would this be a better fix given that data is already being read in
memory
> ?
>
> However it isn't, and such a solution would not scale for large messages.
> What makes you think otherwise? You also commented to me about
MimeMessage
> loading the body into memory. Please see the javadocs for that class;
> specifically the constructor used:
Yes I made a mistake based on dated knowledge. Serge corrected me.
I can see the patch a bit better now.
>
> > B. Problem with ThreadManager. I think it is best to replace scratchpad
> > thread pool with the one from util concurrent library.
>
> What "scratchpad thread pool"? James uses Thread pool management provided
> by Excalibur. Are you proposing that we replace their thread management
> code with new classes derived from Doug Lea's library?
I think we should do that. Replacing thread pool abstraction should be
straight forward. I never understood why the Avalon folks created concurrent
packages when there was a nice standard threading library around.
There have almost always been threading issues in Avalon. There was talk of
getting Doug Lea library under Apache, but for some reason that never
happened.
Thread pool code was part of scratchpad where the Avalon folks had useful
but not fully qualified code. It looks like it has been moved to not sure
where.
>
> What do you mean by this? The ThreadPool and MimeMessageInputStream
changes
> *are* independent of the Watchdog / Scheduler issue.
Had to wade through a alot of code, so could not easily separate. Thanks for
pointing out.
Harmeet
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: Latest diffs, source code
Posted by "Noel J. Bergman" <no...@devtech.com>.
> A. Problem with MimeMessageInputStreamSource.java
> Would this be a better fix given that data is already being read in memory
?
However it isn't, and such a solution would not scale for large messages.
What makes you think otherwise? You also commented to me about MimeMessage
loading the body into memory. Please see the javadocs for that class;
specifically the constructor used:
http://java.sun.com/products/javamail/1.3/docs/javadocs/javax/mail/internet/
MimeMessage.html#MimeMessage(javax.mail.Session,%20java.io.InputStream)
> B. Problem with ThreadManager. I think it is best to replace scratchpad
> thread pool with the one from util concurrent library.
What "scratchpad thread pool"? James uses Thread pool management provided
by Excalibur. Are you proposing that we replace their thread management
code with new classes derived from Doug Lea's library?
> can you point out where the thread pool finally changes have been done.
> Can't see from the diffs and how this is a problem with current system.
finally {
Thread.currentThread().interrupted();
}
Technically, Thread.interrupted() would suffice; currentThread() is
redundant.
> C. Scheduler leak. This can be addressed in different ways. It is being
> disscussed separately. let us keep the memory leak for thread pool and
> MimeMessageInputStream separate.
What do you mean by this? The ThreadPool and MimeMessageInputStream changes
*are* independent of the Watchdog / Scheduler issue.
--- Noel
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: Latest diffs, source code
Posted by Danny Angus <da...@apache.org>.
I remember discussing it, parseing a message into its MIME components is intensive work (I've written a MIME parser) and should be deferred until it is needed, then not repeated.
d.
> -----Original Message-----
> From: Serge Knystautas [mailto:sergek@lokitech.com]
> Sent: 20 October 2002 00:00
> To: James Developers List
> Subject: Re: Latest diffs, source code
>
>
> Yes, you're wrong. The whole point of the class (and related ones) is to
> avoid loading and parsing the message into memory. This improved
> performance by an order of magnitude back when it was added.
>
> Serge Knystautas
> Loki Technologies
> http://www.lokitech.com/
> ----- Original Message -----
> From: "Harmeet Bedi" <ha...@kodemuse.com>
> To: "James Developers List" <ja...@jakarta.apache.org>
> Sent: Saturday, October 19, 2002 9:47 PM
> Subject: Re: Latest diffs, source code
>
>
> > I am suggesting that we keep the byte array in memory for the processing
> of
> > message.
> >
> > Isn't this already done ? I thought the mailets end up reading
> the entire
> > mime message, hence loading the data in at least once. Could be
> wrong, but
> I
> > thought there were places that forced entire message load.
> >
> > Leveraging this info we could remove the temp file. However I admit, it
> has
> > been a long time since I profiled James.
> >
> > Harmeet
>
>
>
> --
> 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: Latest diffs, source code
Posted by Serge Knystautas <se...@lokitech.com>.
Yes, you're wrong. The whole point of the class (and related ones) is to
avoid loading and parsing the message into memory. This improved
performance by an order of magnitude back when it was added.
Serge Knystautas
Loki Technologies
http://www.lokitech.com/
----- Original Message -----
From: "Harmeet Bedi" <ha...@kodemuse.com>
To: "James Developers List" <ja...@jakarta.apache.org>
Sent: Saturday, October 19, 2002 9:47 PM
Subject: Re: Latest diffs, source code
> I am suggesting that we keep the byte array in memory for the processing
of
> message.
>
> Isn't this already done ? I thought the mailets end up reading the entire
> mime message, hence loading the data in at least once. Could be wrong, but
I
> thought there were places that forced entire message load.
>
> Leveraging this info we could remove the temp file. However I admit, it
has
> been a long time since I profiled James.
>
> Harmeet
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: Latest diffs, source code
Posted by Harmeet Bedi <ha...@kodemuse.com>.
I am suggesting that we keep the byte array in memory for the processing of
message.
Isn't this already done ? I thought the mailets end up reading the entire
mime message, hence loading the data in at least once. Could be wrong, but I
thought there were places that forced entire message load.
Leveraging this info we could remove the temp file. However I admit, it has
been a long time since I profiled James.
Harmeet
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: Latest diffs, source code
Posted by "Peter M. Goldstein" <pe...@yahoo.com>.
Harmeet,
Please read the previous emails on this topic. Your summations are
incorrect.
--Peter
> -----Original Message-----
> From: Harmeet Bedi [mailto:harmeet@kodemuse.com]
> Sent: Saturday, October 19, 2002 6:01 PM
> To: James Developers List
> Subject: Re: Latest diffs, source code
>
> There seem to be 3 problems that are being addressed.
>
> A. Problem with MimeMessageInputStreamSource.java. current finalize
may
> not
> fix memory issues if the system is under a lot of stress.
> Would this be a better fix given that data is already being read in
memory
> ?
>
> Index: MimeMessageInputStreamSource.java
> ===================================================================
> RCS file:
> /home/cvs/jakarta-
> james/src/java/org/apache/james/core/MimeMessageInputStrea
> mSource.java,v
> retrieving revision 1.8
> diff -u -r1.8 MimeMessageInputStreamSource.java
> --- MimeMessageInputStreamSource.java 3 Sep 2002 16:52:16 -0000 1.8
> +++ MimeMessageInputStreamSource.java 19 Oct 2002 21:55:55 -0000
> @@ -24,9 +24,9 @@
> public class MimeMessageInputStreamSource extends MimeMessageSource {
>
> /**
> - * A temporary file used to hold the message stream
> + * message stream
> */
> - File file = null;
> + byte[] msgbytes = null;
>
> /**
> * The full path of the temporary file
> @@ -46,32 +46,19 @@
> */
> public MimeMessageInputStreamSource(String key, InputStream in)
> throws MessagingException {
> - //We want to immediately read this into a temporary file
> - //Create a temp file and channel the input stream into it
> - OutputStream fout = null;
> try {
> - file = File.createTempFile(key, ".m64");
> - fout = new BufferedOutputStream(new
FileOutputStream(file));
> + ByteArrayOutputStream baout = new
ByteArrayOutputStream();
> int b = -1;
> - while ((b = in.read()) != -1) {
> - fout.write(b);
> + byte[] ba = new byte[1024];
> + while ((b = in.read(ba)) != -1) {
> + baout.write(ba,0,b);
> }
> - fout.flush();
> - file.deleteOnExit();
> -
> - sourceId = file.getCanonicalPath();
> + msgbytes = baout.toByteArray();
> + sourceId = key;
> } catch (IOException ioe) {
> throw new MessagingException("Unable to retrieve the
data: "
> +
> ioe.getMessage(), ioe);
> } finally {
> try {
> - if (fout != null) {
> - fout.close();
> - }
> - } catch (IOException ioe) {
> - // Ignored - logging unavailable to log this
non-fatal
> error.
> - }
> -
> - try {
> if (in != null) {
> in.close();
> }
> @@ -96,7 +83,7 @@
> * @return a <code>BufferedInputStream</code> containing the data
> */
> public synchronized InputStream getInputStream() throws
IOException {
> - return new BufferedInputStream(new FileInputStream(file));
> + return new ByteArrayInputStream(msgbytes);
> }
>
> /**
> @@ -107,22 +94,6 @@
> * @throws IOException if an error is encoutered while computing
the
> size of the message
> */
> public long getMessageSize() throws IOException {
> - return file.length();
> - }
> -
> - /**
> - * <p>Finalizer that closes and deletes the temp file. Very
bad.</p>
> - * <p>TODO: Should be replaced with a more robust cleanup
> mechanism</p>
> - *
> - */
> - public void finalize() {
> - try {
> - if (file != null && file.exists()) {
> - file.delete();
> - }
> - } catch (Exception e) {
> - //ignore
> - }
> - file = null;
> + return msgbytes.length;
> }
> }
>
>
> B. Problem with ThreadManager. I think it is best to replace
scratchpad
> thread pool with the one from util concurrent library. Also not can
you
> point out where the thread pool finally changes have been done. Can't
see
> from the diffs and how this is a problem with current system.
>
> C. Scheduler leak. This can be addressed in different ways. It is
being
> disscussed separately. let us keep the memory leak for thread pool and
> MimeMessageInputStream separate.
>
> Harmeet
>
>
> --
> To unsubscribe, e-mail: <mailto:james-dev-
> unsubscribe@jakarta.apache.org>
> For additional commands, e-mail: <mailto:james-dev-
> help@jakarta.apache.org>
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: Latest diffs, source code
Posted by Harmeet Bedi <ha...@kodemuse.com>.
There seem to be 3 problems that are being addressed.
A. Problem with MimeMessageInputStreamSource.java. current finalize may not
fix memory issues if the system is under a lot of stress.
Would this be a better fix given that data is already being read in memory ?
Index: MimeMessageInputStreamSource.java
===================================================================
RCS file:
/home/cvs/jakarta-james/src/java/org/apache/james/core/MimeMessageInputStrea
mSource.java,v
retrieving revision 1.8
diff -u -r1.8 MimeMessageInputStreamSource.java
--- MimeMessageInputStreamSource.java 3 Sep 2002 16:52:16 -0000 1.8
+++ MimeMessageInputStreamSource.java 19 Oct 2002 21:55:55 -0000
@@ -24,9 +24,9 @@
public class MimeMessageInputStreamSource extends MimeMessageSource {
/**
- * A temporary file used to hold the message stream
+ * message stream
*/
- File file = null;
+ byte[] msgbytes = null;
/**
* The full path of the temporary file
@@ -46,32 +46,19 @@
*/
public MimeMessageInputStreamSource(String key, InputStream in)
throws MessagingException {
- //We want to immediately read this into a temporary file
- //Create a temp file and channel the input stream into it
- OutputStream fout = null;
try {
- file = File.createTempFile(key, ".m64");
- fout = new BufferedOutputStream(new FileOutputStream(file));
+ ByteArrayOutputStream baout = new ByteArrayOutputStream();
int b = -1;
- while ((b = in.read()) != -1) {
- fout.write(b);
+ byte[] ba = new byte[1024];
+ while ((b = in.read(ba)) != -1) {
+ baout.write(ba,0,b);
}
- fout.flush();
- file.deleteOnExit();
-
- sourceId = file.getCanonicalPath();
+ msgbytes = baout.toByteArray();
+ sourceId = key;
} catch (IOException ioe) {
throw new MessagingException("Unable to retrieve the data: " +
ioe.getMessage(), ioe);
} finally {
try {
- if (fout != null) {
- fout.close();
- }
- } catch (IOException ioe) {
- // Ignored - logging unavailable to log this non-fatal
error.
- }
-
- try {
if (in != null) {
in.close();
}
@@ -96,7 +83,7 @@
* @return a <code>BufferedInputStream</code> containing the data
*/
public synchronized InputStream getInputStream() throws IOException {
- return new BufferedInputStream(new FileInputStream(file));
+ return new ByteArrayInputStream(msgbytes);
}
/**
@@ -107,22 +94,6 @@
* @throws IOException if an error is encoutered while computing the
size of the message
*/
public long getMessageSize() throws IOException {
- return file.length();
- }
-
- /**
- * <p>Finalizer that closes and deletes the temp file. Very bad.</p>
- * <p>TODO: Should be replaced with a more robust cleanup mechanism</p>
- *
- */
- public void finalize() {
- try {
- if (file != null && file.exists()) {
- file.delete();
- }
- } catch (Exception e) {
- //ignore
- }
- file = null;
+ return msgbytes.length;
}
}
B. Problem with ThreadManager. I think it is best to replace scratchpad
thread pool with the one from util concurrent library. Also not can you
point out where the thread pool finally changes have been done. Can't see
from the diffs and how this is a problem with current system.
C. Scheduler leak. This can be addressed in different ways. It is being
disscussed separately. let us keep the memory leak for thread pool and
MimeMessageInputStream separate.
Harmeet
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>