Thank you Keith for tracking down the problem, and I am sorry that you
wasted your precious time because of a bad example.
On 3/17/07, Niklas Therning <[EMAIL PROTECTED]> wrote:
Keith McNeill wrote:
> I sorted out the problem. Which took quite awhile...but I'll get to
> that later. The problem was that in:
>
> org.apache.mina.example.proxy.AbstractProxyIoHandler:
>
> public void sessionClosed( IoSession session ) throws Exception
> {
> if( session.getAttachment() != null )
> {
> ( ( IoSession ) session.getAttachment() ).setAttachment(
> null );
> ( ( IoSession ) session.getAttachment() ).close();
> session.setAttachment( null );
> }
> }
>
> The call to session.setAttachemnt(null) throws an NullPointerException.
>
> In Mina 1.0 the backing Map for Attributes/Attachements is a HashMap.
> In 1.1 it is a ConcurrentHashMap. ConcurrentHashMap throws a NPE
> when one tries to put a null value into the map. HashMaps do not.
>
> There are a few ways to fix this...not sure what the mina folks want
> to do:
>
> - special case nulls in setAttaachement()
> - add a removeAttachement() call, like removeAttribute()
> - change the backing map
1.1 must be compatible with 1.0 (i.e. allow null in call to
setAttachment()) so either the first or last would be the appropriate
alternatives. I'm not sure why 1.1 uses a ConcurrentHashMap when 1.0
uses a HashMap. Since the same Map is used to store all attributes I
guess the use of ConcurrentHashMap could cause other incompatibilities,
e.g. if someone uses null as an attribute name. That would work in 1.0
but not in 1.1. Though null isn't a very good attribute name and I'm
sure no one does that.
HashMap has been replaced with ConcurrentHashMap to introduce more
concurrency for some cases. I didn't know that ConcurrentHashMap
throws a NPE when null value is specified. Let me fix it this Monday.
> This was hard to find because the NPE was being eaten. I added
> logging filters to the app, but didn't do anything with the IO
> adapters. Once I implemented exceptionCaught() in
> AbstractProxyIoHandler, it was easy to find the problem. I think that
> the default action for exceptionCaught should be to report the error
> (in IoHandlerAdapter, and other places). Force people to turn it off
> once they have figured things out. It will make debugging a lot easier.
I think it's weird that the logging filter didn't see the exception. I
think it should have so that could be a bug. Any one else out there who
knows why it doesn't see it?
I guess Neil is saying that he was able to track down the problem by
adding a LoggingFilter. It seems like the proxy example doesn't
override exceptionCaught().
Implementing IoHandlerAdapter.exceptionCaught() to log the exception
can be annoying if a user already added a LoggingFilter, which will
cause duplicate messages.
We could probably use a different log message for
IoHandlerAdapter.exceptionCaught(), which indicates that a user has to
override exceptionCaught() method. For example:
SessionLog.warn(session, "EXCEPTION, Please override
IoHandlerAdapter.exceptionCaught() for proper handling: ", cause);
WDYT?
Trustin
--
what we call human nature is actually human habit
--
http://gleamynode.net/
--
PGP Key ID: 0x0255ECA6
