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/11/09 00:45:19 UTC
[PATCH/VOTE] - MX Chaining fix.
All,
This is a fix for bug #14381. MX Chaining is broken for the reasons
explained in that bug report. This patch separates the connect and the
send stages of the delivery and simplifies a large block of exception
handling code. It's deliberately a fairly minimal change. Were this
earlier in the cycle I'd suggest a more radical code change. But this
one solves the problem and presents minimal risk.
As we are past code freeze, this requires a vote of the committers to be
accepted. I think the problem is severe enough and the fix is localized
enough to merit a patch, so I vote +1. Thoughts?
--Peter
RE: [PATCH/VOTE] - MX Chaining fix.
Posted by "Noel J. Bergman" <no...@devtech.com>.
So you are concerned that there might be a connect exception that isn't due
to a server problem?
--- Noel
-----Original Message-----
From: Serge Knystautas [mailto:sergek@lokitech.com]
Sent: Wednesday, November 13, 2002 0:34
To: James Developers List
Subject: Re: [PATCH/VOTE] - MX Chaining fix.
----- Original Message -----
From: "Noel J. Bergman" <no...@devtech.com>
> > +0.5 on this... I'm a bit leery about changing this logic since javamail
> > and remote servers are a bit finicky, but this seems reasonably minimal
> > as you say.
>
> What risk do you see, with respect to finicky servers? All he appears to
do
> is fail over to another MX record if there is a connection error.
Just odd javamail exceptions that don't make much sense... just haven't
always seen the most clear exceptions, and sometimes that's javamail's fault
and sometimes it's weird servers.
Serge Knystautas
Loki Technologies
http://www.lokitech.com/
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: [PATCH/VOTE] - MX Chaining fix.
Posted by Serge Knystautas <se...@lokitech.com>.
----- Original Message -----
From: "Noel J. Bergman" <no...@devtech.com>
> > +0.5 on this... I'm a bit leery about changing this logic since javamail
> > and remote servers are a bit finicky, but this seems reasonably minimal
> > as you say.
>
> What risk do you see, with respect to finicky servers? All he appears to
do
> is fail over to another MX record if there is a connection error.
Just odd javamail exceptions that don't make much sense... just haven't
always seen the most clear exceptions, and sometimes that's javamail's fault
and sometimes it's weird servers.
Serge Knystautas
Loki Technologies
http://www.lokitech.com/
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: [PATCH/VOTE] - MX Chaining fix.
Posted by "Noel J. Bergman" <no...@devtech.com>.
> +0.5 on this... I'm a bit leery about changing this logic since javamail
> and remote servers are a bit finicky, but this seems reasonably minimal
> as you say.
What risk do you see, with respect to finicky servers? All he appears to do
is fail over to another MX record if there is a connection error.
> I have a small cosmetic change I'd like to patch
> to the code at some point... not sure how formal
> we want to be with the code freeze at this point,
> or if doc adds and the like are to be held off.
Good question. I consider comments to be part of the code. OTOH, we do
want to update the docs.
--- Noel
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: [PATCH/VOTE] - MX Chaining fix.
Posted by Serge Knystautas <se...@lokitech.com>.
Peter M. Goldstein wrote:
> All,
>
> This is a fix for bug #14381. MX Chaining is broken for the reasons
> explained in that bug report. This patch separates the connect and the
> send stages of the delivery and simplifies a large block of exception
> handling code. It's deliberately a fairly minimal change. Were this
> earlier in the cycle I'd suggest a more radical code change. But this
> one solves the problem and presents minimal risk.
>
> As we are past code freeze, this requires a vote of the committers to be
> accepted. I think the problem is severe enough and the fix is localized
> enough to merit a patch, so I vote +1. Thoughts?
+0.5 on this... I'm a bit leery about changing this logic since javamail
and remote servers are a bit finicky, but this seems reasonably minimal
as you say.
Anyway, I know you already committed it (I had glanced it over when you
first sent it and nothing seemed glaringly wrong). I have a small
cosmetic change I'd like to patch to the code at some point... not sure
how formal we want to be with the code freeze at this point, or if doc
adds and the like are to be held off.
--
Serge Knystautas
Loki Technologies
http://www.lokitech.com/
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: [PATCH/VOTE] - POP3 Performance/Functionality Fix.
Posted by simon <si...@sydneybluegum.com>.
Peter, Thank you.
I have downloaded the latest cvs and added your changes and
my test message of 1.5meg is now
downloaded in 3 or 4 seconds compared to 3 minutes and 46 seconds.
My system is Java 1.4.1 on windows 2000. Will move to linux soon.
Had posted a message on the users email list and Noel was going to look
into it.
Would be good for all those windows people having the same problem if
this fix could be included in the next release.
Please post the solution on the on the users list when available.
Thanks again.
Regards,
Simon
Peter M. Goldstein wrote:
>All,
>
>This is a fix for the observed performance problem with POP3 on some
>platform JVM combinations. Basically it amounts to adding a
>BufferedOutputStream around the OutputStream produced by the socket.
>This also seems to resolve this issue encountered by JRC (Randy),
>although the reason for this is less clear.
>
>As we are past code freeze, this requires a vote of the committers to be
>accepted. I think the problem is severe enough (as it is impacting our
>user community) and the fix is localized enough to merit a patch, so I
>vote +1. Thoughts?
>
>--Peter
>
>
>------------------------------------------------------------------------
>
>Index: jakarta-james/src/java/org/apache/james/pop3server/POP3Handler.java
>===================================================================
>RCS file: /home/cvs/jakarta-james/src/java/org/apache/james/pop3server/POP3Handler.java,v
>retrieving revision 1.17
>diff -u -r1.17 POP3Handler.java
>--- jakarta-james/src/java/org/apache/james/pop3server/POP3Handler.java 2 Nov 2002 09:03:52 -0000 1.17
>+++ jakarta-james/src/java/org/apache/james/pop3server/POP3Handler.java 12 Nov 2002 07:24:45 -0000
>@@ -238,7 +238,7 @@
> }
>
> try {
>- outs = socket.getOutputStream();
>+ outs = new BufferedOutputStream(socket.getOutputStream(), 1024);
> out = new InternetPrintWriter(outs, true);
> state = AUTHENTICATION_READY;
> user = "unknown";
>@@ -846,8 +846,6 @@
> writeLoggedFlushedResponse(responseString);
> return;
> }
>- //?May be written as
>- //return parseCommand("TOP " + num + " " + Integer.MAX_VALUE);?
> try {
> MailImpl mc = (MailImpl) userMailbox.elementAt(num);
> if (mc != DELETED) {
>@@ -859,6 +857,8 @@
> theWatchdog,
> theConfigData.getResetLength());
> mc.writeMessageTo(nouts);
>+ nouts.flush();
>+ // TODO: Is this an extra CRLF?
> out.println();
> out.println(".");
> out.flush();
>@@ -888,7 +888,6 @@
> responseString = responseBuffer.toString();
> writeLoggedFlushedResponse(responseString);
> }
>- // -------------------------------------------?
> } else {
> responseString = ERR_RESPONSE;
> writeLoggedFlushedResponse(responseString);
>@@ -935,6 +934,7 @@
> theWatchdog,
> theConfigData.getResetLength());
> mc.writeContentTo(nouts, lines);
>+ nouts.flush();
> out.println(".");
> out.flush();
> } else {
>
>
>
>------------------------------------------------------------------------
>
>--
>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: [PATCH/VOTE] - POP3 Performance/Functionality Fix.
Posted by "Peter M. Goldstein" <pe...@yahoo.com>.
Serge,
> +1 as long as you've got flush's in all the right places.
Yep. We use a BufferedReader everywhere else, so the flushes were
already in place (see some of the late stage bug fixes on POP3 for this
revision). It's only in doTOP and doRETR that we directly access the
underlying OutputStream, and thus only there that additional flush()
calls were required.
--Peter
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: [PATCH/VOTE] - POP3 Performance/Functionality Fix.
Posted by Serge Knystautas <se...@lokitech.com>.
Peter M. Goldstein wrote:
> All,
>
> This is a fix for the observed performance problem with POP3 on some
> platform JVM combinations. Basically it amounts to adding a
> BufferedOutputStream around the OutputStream produced by the socket.
> This also seems to resolve this issue encountered by JRC (Randy),
> although the reason for this is less clear.
>
> As we are past code freeze, this requires a vote of the committers to be
> accepted. I think the problem is severe enough (as it is impacting our
> user community) and the fix is localized enough to merit a patch, so I
> vote +1. Thoughts?
+1 as long as you've got flush's in all the right places.
--
Serge Knystautas
Loki Technologies
http://www.lokitech.com/
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: [PATCH/VOTE] - POP3 Performance/Functionality Fix.
Posted by "Noel J. Bergman" <no...@devtech.com>.
> I accept your logic but I'm extremely wary of introducing bugs at this
stage.
> Any more late changes will get a -0 or even -1 from me on this basis
alone.
I understand, Danny. In this case, the latter patch fixed a bunch of user
complaints. The MX patch could perhaps have been deferred.
> I was under the impression (I may be wrong) that MX priorities exist only
> in order toprovide fail-over and basic load sharing.
Isn't that what Peter's patch did? Provide fail-over?
--- Noel
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: [PATCH/VOTE] - POP3 Performance/Functionality Fix.
Posted by Danny Angus <da...@apache.org>.
+0
To this and peter's previous patch.
I accept your logic but I'm extremely wary of introducing bugs at this stage.
Any more late changes will get a -0 or even -1 from me on this basis alone.
In particular the MX patch could, although I trust you to have got it right, easily lead to unforseen loops or recursions especially as here we are making decisions base on a set of possible responses which we cannot be 100% sure we have fully enumerated. Even if we have identified every possible legal response we may still find illegal responses.
For what its worth I think that handling mail server routing by terminating connections after they have been made and relying on the sender to try another host until one accepts the mail is a pretty poor hack in the first place when compared with using alternatives, like packet filtering to route the connection to the correct host in the first instance, and good old mail forwarding by rules.
I was under the impression (I may be wrong) that MX priorities exist only in order to provide fail-over and basic load sharing.
d.
> As we are past code freeze, this requires a vote of the committers to be
> accepted. I think the problem is severe enough (as it is impacting our
> user community) and the fix is localized enough to merit a patch, so I
> vote +1. Thoughts?
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
[PATCH/VOTE] - POP3 Performance/Functionality Fix.
Posted by "Peter M. Goldstein" <pe...@yahoo.com>.
All,
This is a fix for the observed performance problem with POP3 on some
platform JVM combinations. Basically it amounts to adding a
BufferedOutputStream around the OutputStream produced by the socket.
This also seems to resolve this issue encountered by JRC (Randy),
although the reason for this is less clear.
As we are past code freeze, this requires a vote of the committers to be
accepted. I think the problem is severe enough (as it is impacting our
user community) and the fix is localized enough to merit a patch, so I
vote +1. Thoughts?
--Peter
RE: [PATCH/VOTE] - MX Chaining fix.
Posted by "Noel J. Bergman" <no...@devtech.com>.
I think that a problem description in bugzilla is fine, and it is probably
underused. But code ought to be self-describing, explaining WHY something
is as it is, not just what it does.
It is a shame that the source control and bug tracking systems don't
integrate better. I admit to being out of the habit, but I suppose we can
all try to get (back) into the habit of putting the bug# into the CVS record
when we post a bugfix.
--- Noel
-----Original Message-----
From: Peter M. Goldstein [mailto:peter_m_goldstein@yahoo.com]
Sent: Monday, November 11, 2002 15:12
To: 'James Developers List'
Subject: RE: [PATCH/VOTE] - MX Chaining fix.
Noel,
> I've now read the bug report and the code. Seems reasonable.
> However, there are useful comments in the bug report that
> probably ought to be in the code (or at least in the CVS).
>
> Specifically: "there should be a separate try/catch block around the
> transport.connect() statement, to handle connect errors separately
> from errors that arise during message transmission. Connect errors
> almost always indicates that further SMTP servers associated with
> the MX record should be tried, while errors in message transmission
> are generally protocol-level errors that would occur with any SMTP
> server associated with the MX record (the exception being
> IOExceptions that indicate a failure in the underlying
> transport)" provides more elaboration than the current comments.
>
> I guess I'm not a big fan of having the bug database as the only (or
> best) source for why the code is as it is.
Ok. I thought the comment I dropped in the code was sufficient, but we
can certainly cut and paste this into the code itself.
Personally I think we under use Bugzilla as a repository for useful
information. This is exactly the sort of information a bug tracking
system is supposed to track. :)
--Peter
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: [PATCH/VOTE] - MX Chaining fix.
Posted by "Peter M. Goldstein" <pe...@yahoo.com>.
Noel,
> I've now read the bug report and the code. Seems reasonable.
However,
> there are useful comments in the bug report that probably ought to be
in
> the
> code (or at least in the CVS).
>
> Specifically: "there should be a separate try/catch block around the
> transport.connect() statement, to handle connect errors separately
from
> errors that arise during message transmission. Connect errors almost
> always
> indicates that further SMTP servers associated with the MX record
should
> be
> tried, while errors in message transmission are generally
protocol-level
> errors that would occur with any SMTP server associated with the MX
record
> (the exception being IOExceptions that indicate a failure in the
> underlying
> transport)" provides more elaboration than the current comments.
>
> I guess I'm not a big fan of having the bug database as the only (or
best)
> source for why the code is as it is.
Ok. I thought the comment I dropped in the code was sufficient, but we
can certainly cut and paste this into the code itself.
Personally I think we under use Bugzilla as a repository for useful
information. This is exactly the sort of information a bug tracking
system is supposed to track. :)
--Peter
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: [PATCH/VOTE] - MX Chaining fix.
Posted by "Noel J. Bergman" <no...@devtech.com>.
Peter,
I've now read the bug report and the code. Seems reasonable. However,
there are useful comments in the bug report that probably ought to be in the
code (or at least in the CVS).
Specifically: "there should be a separate try/catch block around the
transport.connect() statement, to handle connect errors separately from
errors that arise during message transmission. Connect errors almost always
indicates that further SMTP servers associated with the MX record should be
tried, while errors in message transmission are generally protocol-level
errors that would occur with any SMTP server associated with the MX record
(the exception being IOExceptions that indicate a failure in the underlying
transport)" provides more elaboration than the current comments.
I guess I'm not a big fan of having the bug database as the only (or best)
source for why the code is as it is.
--- Noel
-----Original Message-----
From: Peter M. Goldstein [mailto:peter_m_goldstein@yahoo.com]
Sent: Friday, November 08, 2002 16:45
To: 'James Developers List'
Subject: [PATCH/VOTE] - MX Chaining fix.
All,
This is a fix for bug #14381. MX Chaining is broken for the reasons
explained in that bug report. This patch separates the connect and the
send stages of the delivery and simplifies a large block of exception
handling code. It's deliberately a fairly minimal change. Were this
earlier in the cycle I'd suggest a more radical code change. But this
one solves the problem and presents minimal risk.
As we are past code freeze, this requires a vote of the committers to be
accepted. I think the problem is severe enough and the fix is localized
enough to merit a patch, so I vote +1. Thoughts?
--Peter
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>