mina-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Maarten Bosteels" <mbosteels....@gmail.com>
Subject Re: PorotocolCodecFilter potential pbs and improvement
Date Fri, 05 Sep 2008 07:59:14 GMT
Hello,

On Wed, Sep 3, 2008 at 8:08 AM, Emmanuel Lecharny <elecharny@gmail.com> wrote:
> Hi,
>
> while adding some Javadoc into this class, I have found that the
> encoder/decoder instances creation might be not thread safe. We are using a
> getDecoder0() where we have such a code :
>
>   private ProtocolDecoder getDecoder0(IoSession session) throws Exception {
>       ProtocolDecoder decoder = (ProtocolDecoder) session
>               .getAttribute(DECODER);
>       if (decoder == null) {
>           decoder = factory.getDecoder(session);
>           session.setAttribute(DECODER, decoder);
>       }
>       return decoder;
>   }
>

AFAICS there is indeed a chance that we call factory.getDecoder()
twice for the same session.
Normally this shouldn't cause too much harm. But people might expect
that getDecoder is only called once per session; not sure that is
documented somewhere.

Side note:  I think it would be great if we had a list with such
guarantees made by MINA.
An example of another guarantee that should be in that list:
 ProtocolDecoder.decoder(IoSession session, IoBuffer in,
ProtocolDecoderOutput out)
 will NEVER be called simultaneously for the same IoSession



> This method is called for every messageReceived() invocation :
>
>   public void messageReceived(NextFilter nextFilter, IoSession session,
>           Object message) throws Exception {
>       if (!(message instanceof IoBuffer)) {
>           nextFilter.messageReceived(session, message);
>           return;
>       }
>
>       IoBuffer in = (IoBuffer) message;
>       ProtocolDecoder decoder = getDecoder0(session);
>       ...
>
> I know it's very unlikely that we receive two messages on the same session
> at the same time, but this can be a possibility, AFAIK. I suggest we
> synchronize this portion of the code this way :
>
>
>   private ProtocolDecoder getDecoder0(IoSession session) throws Exception {
>       // We have to synchronize this section as we may have to create the
> decoder
>       // here on the first invocation.
>       synchronized (factory) {
>           ProtocolDecoder decoder = (ProtocolDecoder)
> session.getAttribute(DECODER);
>
>           if ( decoder == null ) {
>               // No existing instance ? Create it and stores it into the
> session
>               decoder = factory.getDecoder(session);
>               session.setAttribute(DECODER, decoder);
>           }
>                     return decoder;
>       }
>

But why synchronize on the factory instead of on the session ?

> Now, I think we can d something slightly better : we have two methods -
> getDecoder0(IoSession) and getDecoder(IoSession) - doing the exact same
> thing (returning the session decoder), except the fact that getDecoder0()
> inject the decoder into the session, if it was not already done. It makes me
> think that we might want to do taht during the createSession() event
> processing, instead of doing it while processing  the first message
> received.
>
> We will just have to implement the sessionCreated() method, and then we can
> remove the getDecoder0() method, using the getDecoder() method instead,
> without any synchronization.
>
> thoughts ?

I think that will be a nice improvement: simpler and more thread-safe,
don't see any downsides.

Regards,
Maarten

>
> --
> --
> cordialement, regards,
> Emmanuel L├ęcharny
> www.iktek.com
> directory.apache.org
>
>
>
Mime
View raw message