james-server-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Robert Burrell Donkin" <robertburrelldon...@gmail.com>
Subject [JAMESHandler] Streams in AbstractJamesHandler
Date Sat, 05 Apr 2008 09:19:39 GMT
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


Mime
View raw message