james-server-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bernd Fondermann" <bernd.fonderm...@googlemail.com>
Subject Re: [JAMESHandler] Streams in AbstractJamesHandler
Date Sat, 05 Apr 2008 19:04:40 GMT
On Sat, Apr 5, 2008 at 11:19 AM, Robert Burrell Donkin
<robertburrelldonkin@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


Mime
View raw message