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 Robert Burrell Donkin <ro...@gmail.com> on 2008/04/05 11:19:39 UTC

[JAMESHandler] Streams in AbstractJamesHandler

been staring hard at some of the handler framework

from AbstractJamesHandler:

    > /**
   >  * Helper method for accepting connections.
   >  * This MUST be called in the specializations.
   >   *
   >  * @param connection The Socket which belongs to the connection
   >  * @throws IOException get thrown if an IO error is detected
   >  */
   > protected void initHandler( Socket connection ) throws IOException {
   >     this.socket = connection;
#9 >     remoteIP = socket.getInetAddress().getHostAddress();
#9 >     remoteHost = dnsServer.getHostName(socket.getInetAddress());
   >
   >     try {
#1 >         synchronized (this) {
#1 >             handlerThread = Thread.currentThread();
#1 >         }
#6 >         in = new BufferedInputStream(socket.getInputStream(), 1024);
   >         // An ASCII encoding can be used because all transmissions other
   >         // that those in the message body command are guaranteed
   >         // to be ASCII
   >
#6 >         outs = new BufferedOutputStream(socket.getOutputStream(), 1024);
   >         // enable tcp dump for debug
   >         if (tcplogprefix != null) {
   >             outs = new SplitOutputStream(outs, new
FileOutputStream(tcplogprefix+"out"));
   >             in = new CopyInputStream(in, new
FileOutputStream(tcplogprefix+"in"));
   >         }
#8 >         inReader = new CRLFTerminatedReader(in, "ASCII");
   >
#8 >         out = new InternetPrintWriter(outs, true);
#3 >     } catch (RuntimeException e) {
#4 >         StringBuffer exceptionBuffer =
#4 >             new StringBuffer(256)
#4 >                 .append("Unexpected exception opening from ")
#4 >                 .append(remoteHost)
#4 >                 .append(" (")
#4 >                 .append(remoteIP)
#4 >                 .append("): ")
#4 >                 .append(e.getMessage());
#4 >         String exceptionString = exceptionBuffer.toString();
#4 >         getLogger().error(exceptionString, e);
#7 >         throw e;
   >     } catch (IOException e) {
   >         StringBuffer exceptionBuffer =
   >             new StringBuffer(256)
   >                 .append("Cannot open connection from ")
   >                 .append(remoteHost)
   >                 .append(" (")
   >                 .append(remoteIP)
   >                 .append("): ")
   >                 .append(e.getMessage());
   >         String exceptionString = exceptionBuffer.toString();
#2 >         getLogger().error(exceptionString, e);
#7 >         throw e;
   >     }
   >
#5 >     if (getLogger().isInfoEnabled()) {
#5 >         StringBuffer infoBuffer =
#5 >             new StringBuffer(128)
#5 >                     .append("Connection from ")
#5 >                     .append(remoteHost)
#5 >                     .append(" (")
#5 >                     .append(remoteIP)
#5 >                     .append(")");
#5 >         getLogger().info(infoBuffer.toString());
   >     }
   > }

#1 why is this line and this line only synchronised?

#2 logging the full stack trace at error seems like it's going to fill
up the logs pretty quickly if someone tries a DOS. seems better to
WARN with a brief message and then log details at debug.

#3 catching all runtimes seems reasonable to me at this point. anyone
else see any reason not to?

#4 see #2 (but this is more likely to be a coding bug)

#5 bit of a judgement call this one: whether to log at info or debug.
yes, will fill up logs but people probably shouldn't be logging at
info live on the net.

#6 uses Buffered streams for IO.
  6A the buffer size is hard coded. this is ok or does it need to be
configurable?
  6B performance wise, many implementations read character by
character. IIRC Andrew C. Oliver posted about that reading or writing
bytewise from a buffered source is an anti-pattern. can anyone
explain? is buffering the right choice? do implementations need to
fill buffers?

#7 when an exception is caught, it is immediately rethrown. in this
case, are the streams ever closed? do they need to be?

#8 each protocol needs to decide whether to use character or bytewise
streams. so, i think that either in+outs, or out+inReader will be used
but not both. would it be better to split off subclasses or would this
be overengineering?

#9 these fields only seem to be used when logging exceptions. reverse
DNS lookups have a tendency to be expensive and will often not produce
any useful information. this cost is paid every time that a connection
is started. is this worthwhile?

- robert

---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org


Re: [JAMESHandler] Streams in AbstractJamesHandler

Posted by Stefano Bagnara <ap...@bago.org>.
Robert Burrell Donkin ha scritto:
> On Sat, Apr 5, 2008 at 10:19 AM, Robert Burrell Donkin
> <ro...@gmail.com> wrote:
>> been staring hard at some of the handler framework
>>
>>  from AbstractJamesHandler:
> 
> <snip>
> 
>>  #3 >     } catch (RuntimeException e) {
>>  #4 >         StringBuffer exceptionBuffer =
>>  #4 >             new StringBuffer(256)
>>  #4 >                 .append("Unexpected exception opening from ")
>>  #4 >                 .append(remoteHost)
>>  #4 >                 .append(" (")
>>  #4 >                 .append(remoteIP)
>>  #4 >                 .append("): ")
>>  #4 >                 .append(e.getMessage());
>>  #4 >         String exceptionString = exceptionBuffer.toString();
>>  #4 >         getLogger().error(exceptionString, e);
>>  #7 >         throw e;
> 
> <snip>
> 
>>  #7 when an exception is caught, it is immediately rethrown. in this
>>  case, are the streams ever closed? do they need to be?
> 
> when called from handleConnection(Socket), it will be closed on
> handleClean but it's a protected method and so could (potentially) be
> called from subclasses. wonder whether subclasses should really be
> overriding this method.
> 
> - robert

I think I did the last changes to that code, but it was 2 years ago and 
my memory can't even remember what I ate yesterday ;-)

For sure we didn't any "private=>protected" change because of user 
requests, so we can change it if we are not the users of that protected 
method.

 From a fast code read I would say that a "try {} finally { 
cleanHandler(); }" should wrap the initHandler call, but I don't 
remember where we call the handlerConnection and how we handle errors.

I can't find the protected handleClean method you refer to. I only see 
cleanHandler and it is private.

Stefano


---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org


Re: [JAMESHandler] Streams in AbstractJamesHandler

Posted by Robert Burrell Donkin <ro...@gmail.com>.
On Sat, Apr 5, 2008 at 10:19 AM, Robert Burrell Donkin
<ro...@gmail.com> wrote:
> been staring hard at some of the handler framework
>
>  from AbstractJamesHandler:

<snip>

>  #3 >     } catch (RuntimeException e) {
>  #4 >         StringBuffer exceptionBuffer =
>  #4 >             new StringBuffer(256)
>  #4 >                 .append("Unexpected exception opening from ")
>  #4 >                 .append(remoteHost)
>  #4 >                 .append(" (")
>  #4 >                 .append(remoteIP)
>  #4 >                 .append("): ")
>  #4 >                 .append(e.getMessage());
>  #4 >         String exceptionString = exceptionBuffer.toString();
>  #4 >         getLogger().error(exceptionString, e);
>  #7 >         throw e;

<snip>

>  #7 when an exception is caught, it is immediately rethrown. in this
>  case, are the streams ever closed? do they need to be?

when called from handleConnection(Socket), it will be closed on
handleClean but it's a protected method and so could (potentially) be
called from subclasses. wonder whether subclasses should really be
overriding this method.

- robert

---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org


Re: [JAMESHandler] Streams in AbstractJamesHandler

Posted by Robert Burrell Donkin <ro...@gmail.com>.
On Sat, Apr 5, 2008 at 8:04 PM, Bernd Fondermann
<be...@googlemail.com> wrote:
> On Sat, Apr 5, 2008 at 11:19 AM, Robert Burrell Donkin

<snip>

>  >  #4 see #2 (but this is more likely to be a coding bug)
>
>  why do you think so?

RuntimeException are unlikely to propagate this far from the protocol
implementation by intention

- robert

---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org


Re: [JAMESHandler] Streams in AbstractJamesHandler

Posted by Bernd Fondermann <be...@googlemail.com>.
On Sat, Apr 5, 2008 at 10:54 PM, Stefano Bagnara <ap...@bago.org> wrote:
> Bernd Fondermann ha scritto:
>
> >
> > On Sat, Apr 5, 2008 at 11:19 AM, Robert Burrell Donkin
> >
> >
> > >  #2 logging the full stack trace at error seems like it's going to fill
> > >  up the logs pretty quickly if someone tries a DOS. seems better to
> > >  WARN with a brief message and then log details at debug.
> > >
> >
> > +1. plus we don't need the StringBuffer. javac optimizes simple String
> > concatenations.
> > Would make the code a lot easier to read + maintain.
> >
>
>  It is funny to read this from you ;-) [1]
>  AFAIK javac correctly optimizes simple String concatenation using
> StringBuilders since jdk 1.5 [2].
>
>  That said, I think that even in jdk 1.4 and in our specific case (where
> network communication is involved too) modern jvm are optimized enough that
> I prefer pure string concatenation to "hard to read" StringBuffer chains.
>
>  [1] http://www.mail-archive.com/server-dev@james.apache.org/msg11826.html
>  [2]
>  http://www.mail-archive.com/server-dev@james.apache.org/msg11822.html

:-)

  Bernd

---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org


Re: [JAMESHandler] Streams in AbstractJamesHandler

Posted by Stefano Bagnara <ap...@bago.org>.
Robert Burrell Donkin ha scritto:
> On Sat, Apr 5, 2008 at 9:54 PM, Stefano Bagnara <ap...@bago.org> wrote:
>>  Some handler might require checks on the remote hostname. Mailets have
>> access to the remote hostname, too. I guess lazy "resolving" is not an
>> option because it will happen almost always (I've not checked the code, but
>> this is my feeling about this).
> 
> yes, some handlers may need this information but at least one (IMAP)
> does not. if the IP is stored, why isn't lazy resolution an option on
> a per protocol basis?
> 
> - robert

If it works for IMAP then +1.

Stefano


---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org


Re: [JAMESHandler] Streams in AbstractJamesHandler

Posted by Robert Burrell Donkin <ro...@gmail.com>.
On Sat, Apr 5, 2008 at 9:54 PM, Stefano Bagnara <ap...@bago.org> wrote:
> Bernd Fondermann ha scritto:
> > On Sat, Apr 5, 2008 at 11:19 AM, Robert Burrell Donkin

<snip>

> > >  #9 these fields only seem to be used when logging exceptions. reverse
> > >  DNS lookups have a tendency to be expensive and will often not produce
> > >  any useful information. this cost is paid every time that a connection
> > >  is started. is this worthwhile?
> > >
> >
> > AFAIK, dnsServer uses a cache - but the question remains valid.
> > probably, in the end, it is worthwhile - even indespensible - in
> > certain failure situations. ;-)
> >
>
>  Some handler might require checks on the remote hostname. Mailets have
> access to the remote hostname, too. I guess lazy "resolving" is not an
> option because it will happen almost always (I've not checked the code, but
> this is my feeling about this).

yes, some handlers may need this information but at least one (IMAP)
does not. if the IP is stored, why isn't lazy resolution an option on
a per protocol basis?

- robert

---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org


Re: [JAMESHandler] Streams in AbstractJamesHandler

Posted by Stefano Bagnara <ap...@bago.org>.
Bernd Fondermann ha scritto:
> On Sat, Apr 5, 2008 at 11:19 AM, Robert Burrell Donkin
>>  #2 logging the full stack trace at error seems like it's going to fill
>>  up the logs pretty quickly if someone tries a DOS. seems better to
>>  WARN with a brief message and then log details at debug.
> 
> +1. plus we don't need the StringBuffer. javac optimizes simple String
> concatenations.
> Would make the code a lot easier to read + maintain.

It is funny to read this from you ;-) [1]
AFAIK javac correctly optimizes simple String concatenation using 
StringBuilders since jdk 1.5 [2].

That said, I think that even in jdk 1.4 and in our specific case (where 
network communication is involved too) modern jvm are optimized enough 
that I prefer pure string concatenation to "hard to read" StringBuffer 
chains.

[1] http://www.mail-archive.com/server-dev@james.apache.org/msg11826.html
[2]
http://www.mail-archive.com/server-dev@james.apache.org/msg11822.html

>>  #9 these fields only seem to be used when logging exceptions. reverse
>>  DNS lookups have a tendency to be expensive and will often not produce
>>  any useful information. this cost is paid every time that a connection
>>  is started. is this worthwhile?
> 
> AFAIK, dnsServer uses a cache - but the question remains valid.
> probably, in the end, it is worthwhile - even indespensible - in
> certain failure situations. ;-)

Some handler might require checks on the remote hostname. Mailets have 
access to the remote hostname, too. I guess lazy "resolving" is not an 
option because it will happen almost always (I've not checked the code, 
but this is my feeling about this).

Stefano


---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org


Re: [JAMESHandler] Streams in AbstractJamesHandler

Posted by Robert Burrell Donkin <ro...@gmail.com>.
On Sat, Apr 5, 2008 at 8:04 PM, Bernd Fondermann
<be...@googlemail.com> wrote:
>
> On Sat, Apr 5, 2008 at 11:19 AM, Robert Burrell Donkin
>  <ro...@gmail.com> wrote:

<snip>

>  >  #6 uses Buffered streams for IO.
>  >   6A the buffer size is hard coded. this is ok or does it need to be
>  >  configurable?
>
>  I think the default would have been ok here (which is 512 for Sun Java
>  1.4). The advantage of letting the JDK implementation decide is that
>  future machines (under newer JDKs) might be better off with other
>  buffer sizes.
>  The choice people make is probably to achieve that they optimize IP
>  packets going over the wire.
>  Don't know if this can be effectively controlled with the buffer size
>  since the underlying system layers might make their own decisions.
>  But all this is speculation.

yes: better to use the default size

if there's a demand for another buffer size then let the user specify
it in the configuration

>  >   6B performance wise, many implementations read character by
>  >  character.
>
>  "performance wise" in this context means "to degrade performance", right? :-)

not necessarily: some protocols require octet based decision trees so
you pretty much have to read byte-by-byte

>  > IIRC Andrew C. Oliver posted about that reading or writing
>  >  bytewise from a buffered source is an anti-pattern. can anyone
>  >  explain? is buffering the right choice? do implementations need to
>  >  fill buffers?
>
>  doing bytewise IO on an _un_buffered stream is the anti-pattern. so I
>  think buffering is the way to go here.

it's the reading from and writing to the buffers i meant. andrew
opined that  byte-wise reading from and writing to a buffered input
stream is an anti-pattern.

>  >  #8 each protocol needs to decide whether to use character or bytewise
>  >  streams. so, i think that either in+outs, or out+inReader will be used
>  >  but not both. would it be better to split off subclasses or would this
>  >  be overengineering?
>
>  but you could wrap any stream with a reader, couldn't you (via
>  InputStreamReader)? so you could see the stream as the low level
>  choice and the reader as the higher level choice.

i'm not disagreeing about the utility it's just that having all four
accessible from protected methods seems inefficent and potentially a
source of hard to track down bugs with encoding in odd error cases...

>  >  #9 these fields only seem to be used when logging exceptions. reverse
>  >  DNS lookups have a tendency to be expensive and will often not produce
>  >  any useful information. this cost is paid every time that a connection
>  >  is started. is this worthwhile?
>
>  AFAIK, dnsServer uses a cache - but the question remains valid.
>  probably, in the end, it is worthwhile - even indespensible - in
>  certain failure situations. ;-)

even if they are, would it be better to look them up only when needed?

- robert

---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org


Re: [JAMESHandler] Streams in AbstractJamesHandler

Posted by Bernd Fondermann <be...@googlemail.com>.
On Sat, Apr 5, 2008 at 11:19 AM, Robert Burrell Donkin
<ro...@gmail.com> wrote:
> been staring hard at some of the handler framework
>
>  from AbstractJamesHandler:
>
>     > /**
>    >  * Helper method for accepting connections.
>    >  * This MUST be called in the specializations.
>    >   *
>    >  * @param connection The Socket which belongs to the connection
>    >  * @throws IOException get thrown if an IO error is detected
>    >  */
>    > protected void initHandler( Socket connection ) throws IOException {
>    >     this.socket = connection;
>  #9 >     remoteIP = socket.getInetAddress().getHostAddress();
>  #9 >     remoteHost = dnsServer.getHostName(socket.getInetAddress());
>    >
>    >     try {
>  #1 >         synchronized (this) {
>  #1 >             handlerThread = Thread.currentThread();
>  #1 >         }
>  #6 >         in = new BufferedInputStream(socket.getInputStream(), 1024);
>    >         // An ASCII encoding can be used because all transmissions other
>    >         // that those in the message body command are guaranteed
>    >         // to be ASCII
>    >
>  #6 >         outs = new BufferedOutputStream(socket.getOutputStream(), 1024);
>    >         // enable tcp dump for debug
>    >         if (tcplogprefix != null) {
>    >             outs = new SplitOutputStream(outs, new
>  FileOutputStream(tcplogprefix+"out"));
>    >             in = new CopyInputStream(in, new
>  FileOutputStream(tcplogprefix+"in"));
>    >         }
>  #8 >         inReader = new CRLFTerminatedReader(in, "ASCII");
>    >
>  #8 >         out = new InternetPrintWriter(outs, true);
>  #3 >     } catch (RuntimeException e) {
>  #4 >         StringBuffer exceptionBuffer =
>  #4 >             new StringBuffer(256)
>  #4 >                 .append("Unexpected exception opening from ")
>  #4 >                 .append(remoteHost)
>  #4 >                 .append(" (")
>  #4 >                 .append(remoteIP)
>  #4 >                 .append("): ")
>  #4 >                 .append(e.getMessage());
>  #4 >         String exceptionString = exceptionBuffer.toString();
>  #4 >         getLogger().error(exceptionString, e);
>  #7 >         throw e;
>    >     } catch (IOException e) {
>    >         StringBuffer exceptionBuffer =
>    >             new StringBuffer(256)
>    >                 .append("Cannot open connection from ")
>    >                 .append(remoteHost)
>    >                 .append(" (")
>    >                 .append(remoteIP)
>    >                 .append("): ")
>    >                 .append(e.getMessage());
>    >         String exceptionString = exceptionBuffer.toString();
>  #2 >         getLogger().error(exceptionString, e);
>  #7 >         throw e;
>    >     }
>    >
>  #5 >     if (getLogger().isInfoEnabled()) {
>  #5 >         StringBuffer infoBuffer =
>  #5 >             new StringBuffer(128)
>  #5 >                     .append("Connection from ")
>  #5 >                     .append(remoteHost)
>  #5 >                     .append(" (")
>  #5 >                     .append(remoteIP)
>  #5 >                     .append(")");
>  #5 >         getLogger().info(infoBuffer.toString());
>    >     }
>    > }
>
>  #1 why is this line and this line only synchronised?
>
>  #2 logging the full stack trace at error seems like it's going to fill
>  up the logs pretty quickly if someone tries a DOS. seems better to
>  WARN with a brief message and then log details at debug.

+1. plus we don't need the StringBuffer. javac optimizes simple String
concatenations.
Would make the code a lot easier to read + maintain.

>  #4 see #2 (but this is more likely to be a coding bug)

why do you think so?

>  #5 bit of a judgement call this one: whether to log at info or debug.
>  yes, will fill up logs but people probably shouldn't be logging at
>  info live on the net.

agreed.

>  #6 uses Buffered streams for IO.
>   6A the buffer size is hard coded. this is ok or does it need to be
>  configurable?

I think the default would have been ok here (which is 512 for Sun Java
1.4). The advantage of letting the JDK implementation decide is that
future machines (under newer JDKs) might be better off with other
buffer sizes.
The choice people make is probably to achieve that they optimize IP
packets going over the wire.
Don't know if this can be effectively controlled with the buffer size
since the underlying system layers might make their own decisions.
But all this is speculation.

>   6B performance wise, many implementations read character by
>  character.

"performance wise" in this context means "to degrade performance", right? :-)

> IIRC Andrew C. Oliver posted about that reading or writing
>  bytewise from a buffered source is an anti-pattern. can anyone
>  explain? is buffering the right choice? do implementations need to
>  fill buffers?

doing bytewise IO on an _un_buffered stream is the anti-pattern. so I
think buffering is the way to go here.

>  #8 each protocol needs to decide whether to use character or bytewise
>  streams. so, i think that either in+outs, or out+inReader will be used
>  but not both. would it be better to split off subclasses or would this
>  be overengineering?

but you could wrap any stream with a reader, couldn't you (via
InputStreamReader)? so you could see the stream as the low level
choice and the reader as the higher level choice.

>  #9 these fields only seem to be used when logging exceptions. reverse
>  DNS lookups have a tendency to be expensive and will often not produce
>  any useful information. this cost is paid every time that a connection
>  is started. is this worthwhile?

AFAIK, dnsServer uses a cache - but the question remains valid.
probably, in the end, it is worthwhile - even indespensible - in
certain failure situations. ;-)

  Bernd

---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org


Re: [JAMESHandler] Streams in AbstractJamesHandler

Posted by Robert Burrell Donkin <ro...@gmail.com>.
On Sat, Apr 5, 2008 at 9:53 PM, Danny Angus <da...@apache.org> wrote:
> On Sat, Apr 5, 2008 at 10:19 AM, Robert Burrell Donkin
>  <ro...@gmail.com> wrote:
>  >  #6 uses Buffered streams for IO.
>  >   6A the buffer size is hard coded. this is ok or does it need to be
>  >  configurable?
>
>  Its probably OK, assuming that we understand the normal patterns of
>  data we expect to be reading, and we don;t really want to confuse
>  people with too many esoteric config params.
>  OTOH making it configurable would allow people to be optimised for
>  patterns which don't fit our model.

good points, well made

sounds like it might be best to allow subclasses to vary the value (to
allow developers to tune) but not allow users to configure it

- robert

---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org


Re: [JAMESHandler] Streams in AbstractJamesHandler

Posted by Danny Angus <da...@apache.org>.
On Sat, Apr 5, 2008 at 10:19 AM, Robert Burrell Donkin
<ro...@gmail.com> wrote:
>  #6 uses Buffered streams for IO.
>   6A the buffer size is hard coded. this is ok or does it need to be
>  configurable?

Its probably OK, assuming that we understand the normal patterns of
data we expect to be reading, and we don;t really want to confuse
people with too many esoteric config params.
OTOH making it configurable would allow people to be optimised for
patterns which don't fit our model.

>   6B performance wise, many implementations read character by
>  character.

IMHO it depends what your priorities are, if you *need* to clear down
the stream and close it fast, perhaps to release I/O resources you
should have a buffer which will make this happen, i.e. one sized to
accept most of the stream in one go. If you process your bytes slowly
you will fill you buffer, release your resources and then read from
the buffer, but if your processing is quick and your I/O slow you may
just be streaming your bytes straight through the buffer or even
waiting for bytes to be read from the stream. However apart from the
memory overhead I can't see that it would do much harm as long as you
didn't have huge empty buffers hanging about.

But thats just my 2c.


> IIRC Andrew C. Oliver posted about that reading or writing
>  bytewise from a buffered source is an anti-pattern. can anyone
>  explain? is buffering the right choice? do implementations need to
>  fill buffers?

Not Sure what Andy might have been alluding to if its not covered by
my guess work.




>
>  #7 when an exception is caught, it is immediately rethrown. in this
>  case, are the streams ever closed? do they need to be?

Always, in case not closing them locks some I/O resource like a socket
or filehandle.
IMHO you should always close I/O in finally, even if you suspect that
the implementation will release "orphaned" resources at/after garbage
collection.


>  #8 each protocol needs to decide whether to use character or bytewise
>  streams. so, i think that either in+outs, or out+inReader will be used
>  but not both. would it be better to split off subclasses or would this
>  be overengineering?

IMHO choices are specialisation and should always be implemented with
inheritance, however that itself might be more elegantly achieved
using delegation (normalise your model), delegate to an abstract
generalisation and provide alternate implementations which can be
selected at runtime.
Anything else is, to use a datamodelling term again, a "denormalisation".

>
>  #9 these fields only seem to be used when logging exceptions. reverse
>  DNS lookups have a tendency to be expensive and will often not produce
>  any useful information. this cost is paid every time that a connection
>  is started. is this worthwhile?

IMO no, DNS lookups should be off by default and enabled by choice if
the operator is willing to take the performance hit in return for the
convenience of not having to look them up when interpreting the
exception.

d.

---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org